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

Infrastructure: Group Minor/Patch Dependabot updates #3068

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nschonni
Copy link
Contributor

Not sure how the commit message/PR title will show up for this one. The idea is that all the minor patch updates will be in one PR, but the major version updates will continue to be opened as individual PRs

@mcking65 mcking65 requested a review from howard-e July 30, 2024 07:11
Copy link
Contributor

@howard-e howard-e left a comment

Choose a reason for hiding this comment

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

@nschonni interesting!

Not sure how the commit message/PR title will show up for this one.

Ran this on my fork since I was also curious. Here's a PR example with 10 dependencies included. Overall, I like the thought and this would cause a whole lot less 'noise'! There may be minor version bumps which will still require some code modification but figuring out what's changed and pulling that bump into its own PR shouldn't be an issue based on how the grouped PR is presented.

I wonder if there is also a review benefit in grouping together like dependencies which already need (at times) to move forward together, such as eslint*, stylelint*, but also require some code modification at times. Being able to view that isolated but 'grouped' change could be useful but I don't feel strongly. Any thoughts on that?

Also -- if this is merged, it will require someone to enable Grouped security updates under https://github.com/w3c/aria-practices/settings/security_analysis. I don't have access to that.

@howard-e
Copy link
Contributor

howard-e commented Jul 31, 2024

but figuring out what's changed and pulling that bump into its own PR shouldn't be an issue based on how the grouped PR is presented

Looking again at the grouped PR, change notes on lint-staged, prettier and selenium-webdriver were left off at the end. Unsure why.

@nschonni
Copy link
Contributor Author

I wonder if there is also a review benefit in grouping together like dependencies which already need (at times) to move forward together, such as eslint*, stylelint*, but also require some code modification at times. Being able to view that isolated but 'grouped' change could be useful but I don't feel strongly. Any thoughts on that?

Yeah, that might be cleaner, but since the update PRs here aren't usually as urgent to review and land, as with an app, I think it's fine to keep them as the big lump 😄

@nschonni
Copy link
Contributor Author

Looking again at the grouped PR, change notes on lint-staged, prettier and selenium-webdriver were left off at the end. Unsure why.

I wonder if the size of the changelogs might have hit a limit on the API or the bot template

@howard-e
Copy link
Contributor

Looking again at the grouped PR, change notes on lint-staged, prettier and selenium-webdriver were left off at the end. Unsure why.

I wonder if the size of the changelogs might have hit a limit on the API or the bot template

Possibly! I'll check if that's the case

Copy link
Contributor

@howard-e howard-e left a comment

Choose a reason for hiding this comment

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

@nschonni said:

Yeah, that might be cleaner, but since the update PRs here aren't usually as urgent to review and land, as with an app, I think it's fine to keep them as the big lump 😄

Thanks again for this @nschonni. I agree with the above point!

My sole concern on additional details disclosures being left off is because of the 16-bit limit GitHub also applies. There is a Description has been truncated comment attached to the bottom of my example PR which happens because of a PR message max length calculation that's done, the typical max of 65536. Not an issue since the table with the list of repositories is there and knowing which repository to check out is all that's needed (not that there's anything else we can do!)

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.

2 participants