Add automatic closing of major change after their waiting period #2066
Conversation
There was a problem hiding this comment.
The implementation looks reasonable, although if there ever was some "permanent" error in it (e.g. someone removed major_change from the triagebot config of the repo), the job would just get repeated an inifinite number of times. We should probably modify the job to return a more semantic error information, that would distinguish "retry" vs "do not retry". It is done partially already, but the context(...)? calls will just cause a retry.
I like the idea of automating this, but I'd also like to hear from apiraino.
Urgau
force-pushed
the
automate_mcp_acceptence
branch
from
ff92a9d to
28fa3df
Compare
Added a proper error type, with more semantic1 error types. There could still be some errors, like if the repository was deleted (as opposed to a transient error), but I think that could be dealt later. Footnotes
|
Urgau
force-pushed
the
automate_mcp_acceptence
branch
from
28fa3df to
57e94e9
Compare
| if issue | ||
| .labels | ||
| .iter() | ||
| .any(|l| Some(&l.name) == config.concerns_label.as_ref()) |
There was a problem hiding this comment.
So what happens if someone does a second and then someone adds concerns? Is the automatic accept never retried? I wonder if we should also trigger this job once concern(s) are removed and the MCP was already seconded before?
There was a problem hiding this comment.
So what happens if someone does a second and then someone adds concerns? Is the automatic accept never retried?
We get into a weird state, where the job we fired at the second will execute and reject the acceptance, unless the concern was resolved before, which it weird. Worse thing at happens it the MCP is sometimes closed a little early, it's not dramatic, and concerns after second is pretty rare anyway.
I don't really have a great solution for it yet, the only thing I could think of would be to delete the previous jobs for that major change. That would require some big changes, maybe for a follow-up PR.
I wonder if we should also trigger this job once concern(s) are removed and the MCP was already seconded before?
Yes, we should, forgot about that case. Fixed.
Urgau
force-pushed
the
automate_mcp_acceptence
branch
from
57e94e9 to
d751177
Compare
|
I'm gonna go ahead and merge this pull request now, as I have some time now to tests and fix any errors that might occur. Before deploying it in production, I will do a summary and make sure everyone is okay with it. |
| Ok(()) | ||
| } | ||
|
|
||
| async fn schedule_acceptence_job( |
There was a problem hiding this comment.
| async fn schedule_acceptence_job( | |
| async fn schedule_acceptance_job( |
also in other places
| issue | ||
| .post_comment( | ||
| &ctx.github, | ||
| "The final comment period is now complete, this major change is now accepted.\n\nAs the automated representative, I would like to thank the author for their work and everyone else who contributed to this major change proposal." |
There was a problem hiding this comment.
Since this is a bot closing a human-driven process, I would add a bit or wording along the line of "feel free to reopen if there open concerns"
|
Did I miss a discussion about this patch? I ... have written on Zulip in different occasions how I was hesitant to autoclose MCP proposals without a cursory overview to check if a) the discussion settled and b) there were no open concerns (registered or not). Sometimes seconding a proposal arrives too early. I guess at this point we will see if the autoclosing will be too eager. |
|
Sorry, looks like I mentioned your username, but forgot to actually ping you in this comment 🤦 As I knew that you have opinions on this. If it doesn't work in practice, we can always pull it back. |
|
People has expressed multiple times their surprised and frustration that the process isn't more automated. Automating the closing should therefor better align with contributors expectation and reduce frustration (bc of the delay).
To me, this signals that we (T-compiler members) need to be more proactive in registering concerns, not block the 80% other MCP that aren't contentious.
As with And as I said in #2066 (comment), I will a summary and announcement before deploying to the compiler-team repo. |
This patch specifically no, but I mentioned automated closing in #t-compiler > rustbot concern for MCP and issues/PR @ 💬 and no one seemed opposed to it. To be fair, it wasn't the main topic, so not everyone how agreed with |
4 participants
This PR adds automatic closing of major change after their waiting period.
This is made possible by #2022 and #2051.
The way it works is by creating one-shot schedule jobs, based on the (newly added
waiting_periodconfig).I haven't tested it, as it requires postgress, but I intend to test it on this repo before enabling it anywhere else. I should have added sufficient logging to be able to debug any issues that comes up.
cc @apiraino