Skip to content
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

Merged
merged 5 commits into from
Apr 26, 2023

Conversation

Snailedlt
Copy link
Collaborator

@Snailedlt Snailedlt commented Oct 2, 2022

Double check these details before you open a PR

  • PR does not match another non-stale PR currently opened

Features

Fix in-develop labeler by following the steps provided by @Thomas-Boi in #1099

New possible solution: follow the pattern that's being used by our peek-bot:

  1. The pull_request PR saves the PR number/issue number into a file
  2. Upload it as an artifact
  3. Create a workflow-run workflow that runs after the first PR
  4. Get the artifact and label our issue in here where we get full GITHUB_TOKEN access.

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
image of workflow run

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.

@Snailedlt Snailedlt added enhancement devops Use this label for devops related enhancements labels Oct 2, 2022
@Snailedlt Snailedlt requested review from Thomas-Boi, a team, amacado and Panquesito7 and removed request for a team October 2, 2022 15:09
Copy link
Member

@Thomas-Boi Thomas-Boi left a 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:

  1. 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
  1. 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 it devicon-A.
  • Now fork devicon-A to create another forked repo. This is now devicon-B.
  • Make a PR from devicon-B into devicon-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 from devicon-B once it's merged into devicon-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 👍

@Snailedlt
Copy link
Collaborator Author

Snailedlt commented Oct 2, 2022

@Thomas-Boi Thanks for butting in with the review, much appreciated 🙇

  1. I tested this, and it worked just fine, but since I was working on other PR's at the same time, I reset the develop branch after that, so that I could base the new branches on it. That's why it looks like it wasn't tested.

  2. If I understand your explanation correctly, this is exactly what I did. I might have done a bad job explaining it so I'll try again.

    1. I made a feature branch on a fork of my fork (I'll call it org-fork from now on).
    2. Then I made a PR from that org-fork's feature branch into my fork's develop branch.
    3. When that PR was merged, the in_develop_labeler_preflight.yml script ran. Which triggered the in_develop_labeler.yml script.
    4. Both ran successfully (see screenshot in this PR's body), and the issue got labeled (highlighted in the screenshot)

I do see that I forgot to rename the script from in_develop_labeler_preflight.yml to on-develop-pr-merge though, so I guess I'll rename it to that in a new commit. Unless you (or anyone else) has any suggestions for a better name?

TLDR

  • Already done
  • Naming is hard

@Thomas-Boi
Copy link
Member

Thomas-Boi commented Oct 2, 2022

@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 pre-flight is better than on-develop-pr-merge. The second one is not clear that it's specifically for in-develop-labeler. Just a thought: for the other workflows (peek and build) I have a post in the workflows that run afterwards. Since this one run before, maybe just use a pre-in-developer-labeler? It's up to you though. I've considered other naming schemes (pt1 and pt2 for each stage, attaching post to workflows that run after etc.) but none ever seem like the best answer.

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.

@Snailedlt
Copy link
Collaborator Author

Snailedlt commented Oct 2, 2022

@Thomas-Boi
It happens :P I would probably have thought the same thing tbh.

As for the naming convention I actually prefer on-develop-pr-merge for the exact same reason you don't.
That is, that it explains better what it does. Since it's not explicitly for the in-develop-labeler in theory, but just in execution. The only thing it does is upload the pr number as an artifact, which I think will be useful for future jobs/workflows too. One example is for example if we wanted to build and test after a merge. Then we could use the same script as a preflight without renaming it. I guess I could call it something else though, maybe on-develop-pr-merged or on-successful-merge-to-develop

@Snailedlt Snailedlt added the hacktoberfest-accepted Accepted to be counted towards Hacktoberfest label Oct 31, 2022
Copy link
Member

@Panquesito7 Panquesito7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Thanks. 🚀

@Snailedlt Snailedlt added the bug Use this label for pointing out bugs label Jan 11, 2023
Panquesito7
Panquesito7 previously approved these changes Feb 8, 2023
Copy link
Member

@Panquesito7 Panquesito7 left a 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 <76992016+lunatic-fox@users.noreply.github.com>
Copy link
Member

@Panquesito7 Panquesito7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks! 🚀

@Panquesito7
Copy link
Member

Will be merging this for now. If there are any more objections, feel free to create another PR. 🙂

@Panquesito7 Panquesito7 merged commit 70df11e into devicons:develop Apr 26, 2023
@Snailedlt Snailedlt mentioned this pull request Feb 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Use this label for pointing out bugs devops Use this label for devops related enhancements enhancement hacktoberfest-accepted Accepted to be counted towards Hacktoberfest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants