Skip to content

More descriptive PR message when multiple dependencies are fixed in a security update#5595

Merged
Nishnha merged 10 commits intomainfrom
nishnha/transitive-pr-message
Sep 20, 2022
Merged

More descriptive PR message when multiple dependencies are fixed in a security update#5595
Nishnha merged 10 commits intomainfrom
nishnha/transitive-pr-message

Conversation

@Nishnha
Copy link
Copy Markdown
Member

@Nishnha Nishnha commented Aug 29, 2022

This PR adds a #transitive_multidependency_intro method to provide a more descriptive PR intro when multiple dependencies are updated in a transitive dependency update.

Some caveats:


An example PR after this change would read as:

Bumps node-forge to 1.3.1 and updates ancestor dependency webpack-dev-server. These dependencies need to be updated together.

Also see the image below:

Screen Shot 2022-09-15 at 3 13 09 PM

@Nishnha Nishnha force-pushed the nishnha/transitive-pr-message branch 2 times, most recently from 7999e73 to 42ddf8b Compare September 1, 2022 19:54
@Nishnha
Copy link
Copy Markdown
Member Author

Nishnha commented Sep 8, 2022

Another heuristic for transitive dependency updates that @mctofu mentioned is whether the dependencies being updated have a mix of top_level? and non top-level dependencies.

Since it is possible to have a transitive dependency update that only updates the transitive dependency, I think we can get away with checking if any of the dependencies being updated is not a top_level? dep

Checks for a mix of top_level? and !top_level? dependencies to determine whether a PR updates a transitive dependency.

Might be enough to just check for !top_level?
@Nishnha Nishnha force-pushed the nishnha/transitive-pr-message branch from 42ddf8b to 062b377 Compare September 9, 2022 21:13
@Nishnha Nishnha marked this pull request as ready for review September 13, 2022 16:49
@Nishnha Nishnha requested a review from a team as a code owner September 13, 2022 16:49
@Nishnha Nishnha requested review from bdragon and bradify September 13, 2022 19:15
Copy link
Copy Markdown
Contributor

@mctofu mctofu left a comment

Choose a reason for hiding this comment

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

Looks good! Just had some minor cleanup suggestions.

@Nishnha Nishnha merged commit a422d7c into main Sep 20, 2022
@Nishnha Nishnha deleted the nishnha/transitive-pr-message branch September 20, 2022 19:26
@Nishnha
Copy link
Copy Markdown
Member Author

Nishnha commented Sep 20, 2022

I deployed this but haven't been able to generate a PR that uses the updated copy.

Not sure if it's due to how @dependabot recreate works, but I would expect the recreated PR to use the new the PR message.

Oddly enough, the dry-runner is showing the updated copy when I pass in the exact same job, so maybe this will only work for newly created PRs.

@jeffwidman
Copy link
Copy Markdown
Member

Yeah, from #4652:

However, when Dependabot recreated the PR, it didn't reset the commit title or description. It merely reset the code changes of the commit.

Implementation details in #4652 (comment), hopefully we can fix that at some point...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants