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

Raise a single (grouped) PR at most weekly #59

Closed
wants to merge 1 commit into from

Conversation

nicl
Copy link
Contributor

@nicl nicl commented Aug 1, 2023

What does this change?

  • consolidates AWS and non-AWS into a single group
  • adds a global frequency of 7 days to avoid PRs raised more than once a week

This is to balance two competing aims:

  • quick and regular adoption of updates
  • keeping the number of PRs low, to prevent overly burdening teams

Some custom frequency settings are preserved for AWS and Google libraries to reduce noise here.

How to test

run

How can we measure success?

Fewer PRs. Note, Snyk will still alert on any critical security issues.

This change is likely to cut Scala Steward PRs to a most 1/week per configured repo. At the moment it can be 2+ for each.

Have we considered potential risks?

The original rationale for AWS/non-AWS is likely to separate smaller and safer changes (AWS) from others that are likely more important but also more challenging.

In practice, I am not convinced this divide is worth the PR overhead. The vast majority of changes we observe across both groups are non-breaking so a single grouping seems sensible here.

(Caveat: interested in other thoughts here - e.g. @rtyley and @akash1810 when back as somewhat subjective here. I should note though that we in DevX are I think the heaviest users of SS in terms of n. of repos so most impacted by these kinds of changes.)

This is to balance two competing aims:

* quick and regular adoption of updates
* keeping the number of PRs low, to prevent overly burdening teams

As part of this change, the AWS/non-AWS distinction has been
removed, though some custom frequency settings are still applied
to AWS and Google libraries to reduce noise here.
dependencyOverrides = [
{
dependency = { groupId = "com.amazonaws" },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed as it now matches the default.

@@ -1,15 +1,14 @@
# At most we want pull requests for a dependency/group no more than once a week.
# This is to balance responsiveness with dependency management fatigue.
pullRequests.frequency = "7 days"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this works with groupings as expected. If not, the alternative solution is to simply schedule the workflow to run only once a week.

@rtyley
Copy link
Member

rtyley commented Aug 1, 2023

interested in other thoughts here - e.g. @rtyley and @akash1810 when back as somewhat subjective here.

Thanks @nicl - I would like to make some counter-arguments, but I'm quite tired right now, so could you give me a day or two to muster my thoughts?

@shtukas
Copy link

shtukas commented Aug 2, 2023

Haya. Just my 2c. I quite like the AWS and non-AWS ones being separate. For two reasons:

  1. The AWS ones are usually less difficult to review
  2. That one underlying upgrade fails would invalidate the entire grouped PR, which results in far more work, since we would then need to release them one by one :)

@rtyley
Copy link
Member

rtyley commented Aug 11, 2023

I've just gone holiday, but I owe you a reply! So quickly, I think:

  • pullRequests.frequency config doesn't work if the Scala Steward run has failures #60 plays is significant, it's been causing excessive PRs for the past 3 weeks, which has probably exacerbated this issue from DevX's perspective - it's not ideal, but perhaps DevX could have a weekly item on their checklist to check Scala Steward runs are not failing, and thereby ensure that frequency config is effective, with PRs not raised more rapidly than specified?
  • Different teams have different attitudes to Scala Steward PRs - I like to think of them as informational ("here's what you could upgrade, and whether that would pass tests") and a convenient and rapid way of adopting library updates when, eg, a new version of a Guardian library is released (eg, I don't want to wait a week for Scala Steward to raise PRs when a new version of the FAPI client is released) - it's not awful if the PRs stick around for a while before they're merged. In DevX, where PRs are announced by a chatbot, and I think the team intention is to ensure any PR raised is quickly merged, Scala Steward PRs are seen in a different light, and there is obviously a desire to reduce them further that may not actually be felt by other teams (@SHession mentioned the other day that he'd be interested in having more granularity, with separate groups for 'patch' vs 'minor' updates).
    • Could these PRs be less distracting for DevX if the chatbot was configured to not announce PRs by Scala Steward?
  • small point, but I would be surprised if DevX actually is the owner of the most repos tended by Scala Steward (there are 46 public ones, and 14 private ones - does DevX have the most?!)

@rtyley
Copy link
Member

rtyley commented Dec 7, 2023

I'm closing this for now, happy to talk further about it in the future!

@rtyley rtyley closed this Dec 7, 2023
@rtyley rtyley deleted the nicl/blanket-min-7-day-frequency branch January 16, 2025 13:43
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