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

Post Rollup Warnings #1657

Open
rylev opened this issue Sep 30, 2022 · 2 comments
Open

Post Rollup Warnings #1657

rylev opened this issue Sep 30, 2022 · 2 comments
Labels
enhancement New feature or request

Comments

@rylev
Copy link
Member

rylev commented Sep 30, 2022

When creating rollups some PRs are naturally riskier than others. If the rollup introduces bugs or performance issues it can be helpful to get a hint at what PRs might be causing an issue and which PRs can most likely be ignored.

With this in mind, I propose adding support to triagebot for listening for rollup PRs and posting a message with the results of inspecting how risking the rollup is. It could use simple heuristics to begin with.

Configuration Changes

The following would be added to the triagebot.toml:

[rollup-warnings]
trigger_files = [
  "compiler/**/*.rs" # warn on any Rust files in the compiler directory
  # ... more patterns here
]

Warning

For any rolled-up PR included in a rollup that has a file change identified by the patterns in trigger_files, a warning would be posted that looks like the following:

⚠Vulnerable PRs

This rollup has 2 vulnerable PRs.

The following PRs may be particularly vulnerable to breakage:

  • #NNN (10 vulnerable files changed)
  • #NNN (7 vulnerable files changed)

If this rollup introduces any type of regression, you may wish to investigate these PRs first.

Trigger Rate

Looking at some recent rollups, I expect this warning to show the following results:

Future extensions and open questions

  • The mechanism proposed here is very naive. There are many changes that we probably shouldn't consider as vulnerable (e.g., changes to comments). Should we include that in an initial version?
  • Some changes are likely a bad idea to have in a rollup. For example, in rust-lang/rust, a PR that changes over 50 files in compiler should probably not have been rolled up (or at the very least, a rollup should not contain two such PRs). Perhaps a strong waning in such cases would be useful?
@jyn514
Copy link
Member

jyn514 commented Oct 2, 2022

  • Some changes are likely a bad idea to have in a rollup. For example, in rust-lang/rust, a PR that changes over 50 files in compiler should probably not have been rolled up (or at the very least, a rollup should not contain two such PRs). Perhaps a strong waning in such cases would be useful?

I'm not sure this is a useful metric ... a PR that changes that many files is probably mass-renaming a common function, which is actually quite safe. Lines of code is a little better, but can be fooled by the same sort of change - lines of code within a file is more reliable, but can be tricked by moving code from one file to another. I think this is going to be really hard to automate in any useful way.

@jyn514
Copy link
Member

jyn514 commented Oct 2, 2022

I don't think we should add any automation here until we agree what would actually be useful; or at least, agree that it's ok to turn it off if we're not finding it useful.

@ehuss ehuss added the enhancement New feature or request label Jul 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants