Skip to content

Allow configuring Pull Requests to be 'draft' #3628

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

Merged

Conversation

rtyley
Copy link
Contributor

@rtyley rtyley commented Apr 22, 2025

This addresses one of the small remaining problems with handling #3612 - if we're raising a large number of PRs for a preview version of a library, we don't want those PRs to immediately trigger review-requests for all the maintainers of those repos.

If the PRs are raised in draft, then we can see the results of CI on the PRs, and manually promote them to non-draft PRs for review later.

With this PR, the desired behaviour can be configured in .scala-steward.conf, like this:

# pullRequests.draft will ensure that PRs are raised as 'draft' PRs if the forge supports it.
# This is useful if you don't want immediate review requests going to all watchers of the repo.
# Defaults to false (PRs are not raised in draft).
pullRequests.draft = true

Initial support for creating draft PRs in Scala Steward was added with #1961 in February 2021, tho' this was specific to PRs changing the Scala version, rather than updating dependencies, and as support for updating Scala version in Scala Steward improved, the raise-in-draft behaviour was removed with #2081 in April 2021.

PR prepared with @JamieB-gu!

@rtyley rtyley force-pushed the allow-draft-prs-to-be-configured branch from c302d88 to a9f05e3 Compare April 22, 2025 16:36
This addresses one of the small remaining problems with handling
scala-steward-org#3612 .

Co-authored-by: Jamie B <[email protected]>
@rtyley rtyley force-pushed the allow-draft-prs-to-be-configured branch from a9f05e3 to 45087df Compare April 22, 2025 16:47
Copy link

codecov bot commented Apr 22, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.83%. Comparing base (d09e9fc) to head (45087df).
Report is 12 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3628   +/-   ##
=======================================
  Coverage   89.83%   89.83%           
=======================================
  Files         174      174           
  Lines        5045     5045           
  Branches      488      431   -57     
=======================================
  Hits         4532     4532           
  Misses        513      513           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@rtyley rtyley marked this pull request as ready for review April 22, 2025 17:01
@fthomas
Copy link
Member

fthomas commented Apr 24, 2025

Thanks for the PR, @rtyley and @JamieB-gu!

Using a boolean for pullRequests.draft is fine with me but wrt to #3612 I wonder if it would better suit your needs if pullRequests.draft would be a list of UpdatePattern so that draft PRs are only used for updates matching a pattern in that list. Or maybe instead of using a boolean for all PRs we could have a boolean like pullRequests.preReleasesAsDrafts so that draft PRs are only used for the updates matching updates.allowPreReleases. Both of these would prevent Scala Steward from creating draft PRs for normal regular updates. With preReleasesAsDrafts you would also only need to change updates.allowPreReleases for #3612 and not flip pullRequests.draft from false to true and back.

@fthomas
Copy link
Member

fthomas commented Apr 24, 2025

I guess with preReleasesAsDrafts you probably wouldn't need to change your repo config often. You could set it

pullRequests.preReleasesAsDrafts = true
updates.allowPreReleases = [
  { groupId = "com.gu" },
  # all other groupIds of your internal libraries
]

and then get draft PRs for Guardian pre-releases and regular PRs for everything else.

@rtyley
Copy link
Contributor Author

rtyley commented Apr 24, 2025

I wonder if it would better suit your needs if pullRequests.draft would be a list of UpdatePattern so that draft PRs are only used for updates matching a pattern in that list. Or maybe instead of using a boolean for all PRs we could have a boolean like pullRequests.preReleasesAsDrafts so that draft PRs are only used for the updates matching updates.allowPreReleases. Both of these would prevent Scala Steward from creating draft PRs for normal regular updates. With preReleasesAsDrafts you would also only need to change updates.allowPreReleases for #3612 and not flip pullRequests.draft from false to true and back.

Actually, for #3612, we've already decided we'll have to specifically generate a scala-steward.conf for whichever update we want to push out - because we already have a quite extensive scala-steward.conf in global use, and the rules in there clash with the actual minimal configuration we want this specific Scala Steward job to run with, which would be like this:

updates.allow = [{ groupId = "com.gu", artifactId = "redirect-resolver", version = "0.0.38-PREVIEW.try-out-tool-versions-support.2024-04-26T1649.b33d9ccd" }]
commits.message = "[TEST - please ignore]: Update ${artifactName} from ${currentVersion} to ${nextVersion}"

The clashes with elements in our normal config include:

  • pullRequests.frequency & dependencyOverrides.pullRequests.frequency - we normally throttle the rate at which new PRs are raised to avoid overloading the teams that maintain repos. However, this is really unhelpful if we are trying to push out a preview of a new library release - we want all those draft PRs raised now - and not be 'ignored' for days by Scala Steward due to our normal config.
  • pullRequests.grouping - we have a dedicated grouping for our dependencies (under the com.gu groupId), as they're generally more pertinent and important to our teams. However, when we are pushing out a preview of a new library release, we don't want it to interact with any existing PRs under that grouping- we want our own isolated PRs.

As we already have to generate a specific scala-steward.conf, it's no problem for us to have to add or remove a pullRequests.draft flag from the configuration!

Just to be clear, we anticipate having a normal scheduled Scala Steward job running with our normal scala-steward.conf, and then additionally ad-hoc runs of our Scala Steward workflow running with minimal custom-generated scala-steward.conf to perform specific updates as required by developers.

It's also possible, that although I'm focussing on the preview-of-a-new-library-release use-case, we might also want to raise draft PRs for non-preview releases - it's often the case that updating a dependency version number is not enough to get an update PR to compile - manual code updates may be required - and we might want to put that work on the developer of the update, rather than the teams maintaining the consuming repos, so draft PRs would still be appropriate until the PR is in a state fit for maintaining teams to review. Consequently we would probably prefer pullRequests.draft to pullRequests.preReleasesAsDrafts?

Copy link
Member

@fthomas fthomas left a comment

Choose a reason for hiding this comment

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

Thanks for the detailed explanation @rtyley! The simple pullRequests.draft boolean (as in this PR) seems like a good fit for what you're trying to achieve.

@rtyley
Copy link
Contributor Author

rtyley commented Apr 29, 2025

Thanks for approving the PR @fthomas ! Is it ok to also merge at this point? I don't have merge permissions in this repo!

@fthomas fthomas merged commit deebbf4 into scala-steward-org:main May 7, 2025
10 checks passed
@fthomas fthomas added enhancement New feature or request cat:repo-config labels May 7, 2025
@fthomas fthomas added this to the 0.34.0 milestone May 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cat:repo-config enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants