-
Notifications
You must be signed in to change notification settings - Fork 2.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bugfix/fix in develop labeler #1410
Bugfix/fix in develop labeler #1410
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Snailedlt,
I looked through your code and everything looks good. I double checked your testing PR/Issue as well and I would like to remark:
- While PR and Issues are treated the same by GitHub, it might be a good idea to test the whole thing as a PR. Sometime, GitHub Actions differentiate them in unintentional ways. I'd recommend:
- Make a PR into
develop
of the forked repo - See if the PR gets labelled as
in-develop
- Beside testing the above, also test it using a forked repo of the forked repo. This will emulate the scenario that a contributor would be in when they open a PR. What this means in practice:
- You currently have a forked repo of
devicon
. Call itdevicon-A
. - Now fork
devicon-A
to create another forked repo. This is nowdevicon-B
. - Make a PR from
devicon-B
intodevicon-A
. This will create the cross repo environment that causes the original script to fail in the first place. Thus, if your script works, it should label the PR fromdevicon-B
once it's merged intodevicon-A
.
Let me know if the above instructions are too complicated and I can try to explain it better. Other than that, the code looks good and I'm happy to see that someone finally get around to fixing this bug 👍
@Thomas-Boi Thanks for butting in with the review, much appreciated 🙇
I do see that I forgot to rename the script from TLDR
|
@Snailedlt sweet! Thanks for the explanation. I misunderstood the test PR and thought that it's a PR in the same branch. I'm glad that you already test the cross-repo situation 👍. As for the naming scheme, I think the current name Great work SnailedIt 😄 P.S: I'd approve the PR but I think I'll wait after the naming commits just so no one accidentally merge the PR prematurely. |
@Thomas-Boi As for the naming convention I actually prefer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! Thanks. 🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 once the reviews above are addressed.
Co-authored-by: Josélio Júnior <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks! 🚀
Will be merging this for now. If there are any more objections, feel free to create another PR. 🙂 |
* fix issue not being labeled with in-develop * restrict to only run on merge to develop * Remove Python setup Co-authored-by: Josélio Júnior <[email protected]> --------- Co-authored-by: Josélio Júnior <[email protected]> Co-authored-by: David Leal <[email protected]>
Double check these details before you open a PR
Features
Fix in-develop labeler by following the steps provided by @Thomas-Boi in #1099
This PR closes #1099
Notes
This needs to be merged to master in order to work, since it uses
workflow_run
which runs a github actions workflow from the master branch, and the master branch does not yet contain that workflow. Therefore I suggest merging this to develop just before the next release.The bugfix has been tested on my forks by adding it to master, and then creating a pr from a different fork. The PR can be found here, with the accompanying actions run here, which successfully labeled the following issue: Snailedlt#17
NB!
This PR is missing one commit compared to the one I had in my fork. In order to test I had to change the base_url from devicons/devicon to my fork snailedlt/devicon, after the test I removed that temp commit via an interactive rebase and force pushed.