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

CI: Assign milestone on merged PRs #4414

Merged
merged 19 commits into from
Oct 15, 2024
Merged

CI: Assign milestone on merged PRs #4414

merged 19 commits into from
Oct 15, 2024

Conversation

echoix
Copy link
Member

@echoix echoix commented Sep 29, 2024

Addresses #4398 (comment)

This PR assigns a milestone when a PR is closed if none is set, using pull_request_target trigger with closed activity type. It uses up an API call to know if the PR has a milestone set (limit is 5000 per hour when authenticated). If I used the info from the github event context, the information is populated when the job is queued, and will maybe not represent the state once it will run, and will keep the old state if a rerun is made. I tried to use the gh cli instead of their javascript action's ecosystem so steps could be reproduced locally too using the same gh cli calls.

If there are no milestones for that PR, then it uses a second API call to fetch the include/VERSION file at the ref of the merged result (so pull_request_target doesn't matter here, it is still up-to-date). It parses that file like for the coverity scan workflow, but also removes "dev" and "RC" followed by 0 or more numbers. This builds the milestone title to apply, using a third API call.

Using the VERSION file allows it to keep working on newer and older versions (working on older branches if the workflow is backported)

Known limitations:

  • No attempt is made to try to prevent running on forks that merges PRs in their forks: This also allows to change the repo's name and organization in the future without having to change release branches.
  • No attempt to create a milestone if it doesn't exist: this workflow will simply fail, as the gh cli will say that the milestone doesn't exist. As it might only happen once or twice during a year (assuming the milestone wasn't set on the PR already), I didn't take more time to do it. In part because running on PR merges doesn't have a lot of visibility (doesn't show on the pushed commit on main checks for example), and what dates and description should be set? That's more of a manual thing to do.

@echoix echoix requested a review from neteler September 29, 2024 13:33
@github-actions github-actions bot added the CI Continuous integration label Sep 29, 2024
@neteler
Copy link
Member

neteler commented Sep 30, 2024

Thanks for your efforts, @echoix .
This is a rather complex approach which may require others to review it.

(in my original request, I was thinking of graying out the "merge" button until a milestone is manually assigned)

@echoix
Copy link
Member Author

echoix commented Sep 30, 2024

Ah, that's another way, but I didn't understand it was that that you meant. (It has the disadvantage that we would be waiting for the GitHub Action queue for every PR, unless we don't set it as required, and it won't help at all). At least with this PR, PRs can stay a long time before getting merged.

@echoix echoix requested a review from wenzeslaus October 1, 2024 18:52
@echoix
Copy link
Member Author

echoix commented Oct 7, 2024

Can someone take a look at this? It is what I think is the most appropriate solution, covering most cases I could think of, that allows us to never need to set milestones on PRs, while still allowing us to set them manually when we want to.

It is in essence quite simple, but is a bit longer in order to really « show its work » in the steps outputs, so it isn’t a black box.

Copy link
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

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

I'm hopeful that this does what's promised in the description.

But also, I don't like how complex it is. The required milestone approached by @neteler seems like simpler and more aligned with what we do for titles.

@echoix
Copy link
Member Author

echoix commented Oct 14, 2024

I'm hopeful that this does what's promised in the description.

But also, I don't like how complex it is. The required milestone approached by @neteler seems like simpler and more aligned with what we do for titles.

The problem with that is that in cases where the CI has a long queue, it will be slowing us down, as we would be waiting on it.

And it still requires the work of always adding the milestone, instead of just needing it when the current milestone isn't the correct one.

Copy link
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

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

Okay, let's try that.

@echoix echoix added the backport to 8.4 PR needs to be backported to release branch 8.4 label Oct 15, 2024
@echoix echoix merged commit 1adbd2c into OSGeo:main Oct 15, 2024
27 checks passed
@echoix
Copy link
Member Author

echoix commented Oct 15, 2024

(Let the action set the milestone, once it gets its turn in the queue)

@github-actions github-actions bot added this to the 8.5.0 milestone Oct 15, 2024
@echoix
Copy link
Member Author

echoix commented Oct 15, 2024

Works as tested and expected

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport to 8.4 PR needs to be backported to release branch 8.4 CI Continuous integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants