glacier: Append random sequence to branch name to make it unique #1041
Conversation
775f20f
to
d09920f
Compare
|
I'd prefer we use the API to delete the old branch or something like that instead. |
|
Ideally that's what we'd do, but it's not necessarily easy to figure out when the branch should be deleted. We'd need to listen to the PR events and only delete it when the PR is closed or merged. I think it's better to just go with something simple for now so that it's not a pain if you make a typo when invoking the command. |
Sometimes the user (me) will make a mistake (e.g. leave out `/rust-play/` from the URL) when invoking `@rustbot glacier` that means the automatically-created PR has to be closed. However, there was no way to try again, because the branch name would conflict and triagebot would crash. Now, we append an 8-character random alphanumeric string to the branch name so that the branch names are unique and you can try again. This adds a dependency on `rand`, which is probably the most downloaded crate all-time, but we already depended on it through other crates.
d09920f
to
689c731
Compare
|
I mean that we'd always delete it or "force push" or whatever to the branch? Like, I'd want repeated invocations to update the open PR ideally |
|
Hmm... that seems like it might be confusing. I also think it would require significant changes to the current code, and I don't know octocrab etc. well enough to make those changes. |
|
What about creating the file and the pull request as currently done if the branch was created successfully (i.e. it didn't exist), and just updating the file if the branch already exists? See https://docs.rs/octocrab/0.12.0/octocrab/repos/struct.RepoHandler.html#method.update_file |
That could work, but probably triagebot should only do that if there's no PR open for that branch (i.e., the PR was closed). Otherwise there could be confusion if two people do |
Didn't think of that, but what is the probability that it will happend ? Especially since only a limited number of people are allowed to issue the command ? Which is better ? Having to close the PR to fix it or avoiding that potential confusion ? What about duplicates PR for the same ICE ? |
|
Error: Parsing glacier command in comment failed: ...'acier with' | error: invalid link - must be from a playground gist at >| ' different'... Please let |
|
P.S. ok, definitely need to ignore commands in quoted text. I'll try to implement that |
Personally, I think it would be best to start out by only updating the branch if there's no open PR, and then we can always lift that restriction in the future if it causes problems. |
|
Error: Parsing glacier command in comment failed: ...'acier with' | error: invalid link - must be from a playground gist at >| ' different'... Please let |
|
As you see fit. The question then is what to do when the a PR is open ? Emit a warning / error ? Or open a concurrent PR ? I still think it's unlikely that two persons issue the command with different MCVEs but for same ICE / on the same issue. Anyway… |
It's probably not a big deal. I guess I'm just thinking of if someone tried to spam the open PR, but it's probably unlikely. So maybe updating the branch even if a PR is open is fine. Maybe @Mark-Simulacrum has thoughts on what makes sense here? |
|
Spam is not something we're trying to solve too much. I think either updating the branch in place or generating a new branch and updating that will both be fine. I think generally leaning towards one PR per invocation is probably better; PRs are relatively cheap and if need be could also be edited in place by reviewers. |
|
It might be a little cleaner to associate the branch with a single (user, issue) pair and update that in place to provide an easy way to amend PRs, perhaps. But either way seems ok. |
That sounds like a good idea. E.g., if I used it on issue 123, the branch would be named |
|
Right. We'd still need the create if not exists & update logic, but hopefully that's not too hard to pull off. |
Sometimes the user (me) will make a mistake (e.g. leave out
/rust-play/from the URL) when invoking@rustbot glacierthat means the automatically-created PR has to be closed.
However, there was no way to try again, because the branch name
would conflict and triagebot would crash. Now, we append an
8-character random alphanumeric string to the branch name so that
the branch names are unique and you can try again.
This adds a dependency on
rand, which is probably the most downloadedcrate all-time, but we already depended on it through other crates.
r? @Mark-Simulacrum