-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
CI Removes check milestone workflow #19843
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 Removes check milestone workflow #19843
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.
Thanks @thomasjpfan . I agree with all of the above. Similar points were noted by Joel and I in the original issue #19596
This is not a problem: the check can be triggered at any synchronization and, generally pull requests are not finished when the horizon of a milestone arises.
I trust contributors understanding: the true frustrating experience is when your pull request is all green and you wait for months to be reviewed. We had more questions about why reviewer are not answering with respect to "why the changelog is red", for after answering thrice, people understand that this is just a technical check, not a human problem
You can wait till the end and apply the milestone last minute if you want
Don't underestimate maintainers
See above
See point 1.
I'm in the process of automating backport to minor releases if I can't trust the milestone this automation is meaningful. |
Reverting. However, I think that we need to automate more the release process and the cherry-picking + in bug fix release is something that would be nice to have. As I said there #19833 (comment), I think that it should be possible to bypass these GitHub actions during "reviews required", "draft" or "WIP" PRs status (using branch protection, PRs title, and PRs status). It would make it less intimidating than a failure when opening your first PR. IMO, point 3. is a blocker to me while I could live with a default strategy in 2. even if it requires moving to the next milestone at the time of the release. This might be part of the release process to decide to postpone or include a feature. |
I agree with you that this is a frustrating experience. I definitely appreciate the effort towards having more responsive review cycles, but I don't think this check is a good solution for that. Making milestones mandatory will not change reviewers behaviors. Perhaps I should, but I personally never look at milestones unless we're in the last few weeks/days before a release, when things need to get in. Even when I was paid to work on scikit-learn full-time, I had too much on my plate to worry about milestone labels. I assume I'm not the only one. @cmarmo, I'm still unclear on how this check will help the release process (I never took part of the release process so I'm missing some context). If what we care about here is about milestones for merged PR, as opposed to open PRs, then perhaps a solution would be to start adopting an oncall rotation where each week, one of us is responsible of making sure that the merged PRs have correct tags (triage + core devs). Another option is to have a check that ensures those things when merging, instead of something that's part of the CI - not sure if that's technically doable though. I'm skeptical about adding more CI checks in general, and I think we need to be extremely careful about the CI. A flaky / difficult CI is a major source of frustration for both contributors and maintainers. We should only allow the CI to go red for very good reasons. Failing tests and build issues are such good reasons, but a missing / incorrect label doesn't seem like one. Also, when the CI is failing, users should be able to a) understand the issue and b) be able to fix it themselves. This check does not allow that. |
This is not technically doable for now, unless you block the merge protecting the branch: this will show a big red flag anyway so I don't see the difference. About how this check can help the release and the changelog: I was planning to open pull requests for that, where I would have been glad to discuss the follow-up: this is a technical discussion and I think it is more relevant if an implementation is available. This revert just shut me up. |
Reference Issues/PRs
Reverts #19833
What does this implement/fix? Explain your changes.
There is no way to trigger a
pull_request
target with amilestoned
event: https://github.community/t/feature-request-add-milestone-changes-as-activity-type-to-pull-request/16778/4. This means a maintainer would need to add a milestone and add a label to trigger the milestone check. If there is no appropriate label, to add we need to add a label and then remove it to trigger the milestone check.Given that this check is for milestones and we can not trigger it with a change in milestones, I think it is better to remove.
Any other comments?
I think it is good to put items on the 1.0 checklist, but forcing all PRs to have a milestone does not make project management easier.
CC @cmarmo @glemaitre @ogrisel