Skip to content

[Updater] Wire up and flesh out branch naming for Grouped Update PRs#7084

Merged
brrygrdn merged 5 commits intomainfrom
brrygrdn/finalise-dependency-group-branch-naming
Apr 14, 2023
Merged

[Updater] Wire up and flesh out branch naming for Grouped Update PRs#7084
brrygrdn merged 5 commits intomainfrom
brrygrdn/finalise-dependency-group-branch-naming

Conversation

@brrygrdn
Copy link
Copy Markdown
Contributor

We have spiked out using DependencyGroup in branch naming but in our internal tests we are overriding the prefix value for projects in order to work around potential naming collisions since the prototype cannot yet manage superseding of existing pull requests.

Before we go into the next round of testing, we should make our branch naming for grouped PRs more compatible with existing automation and also avoid problems of the branch name overflowing due to a larger-than-normal number of dependencies being involved.

This PR finishes off our grouped naming strategy so it incorporates the normal prefix, the package manager, directory and target branch along with the actual group name being used.

@brrygrdn brrygrdn requested a review from a team as a code owner April 14, 2023 12:15
@brrygrdn brrygrdn force-pushed the brrygrdn/finalise-dependency-group-branch-naming branch 3 times, most recently from 28f7a77 to ca5705c Compare April 14, 2023 14:22
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: I think it would make sense to update GROUP_NAME_PLACEHOLDER here to "all-dependencies". Otherwise branch names will pop up with a *

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good spot, yep, we should be consistent

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah, I've actually realised we don't use this name to generate the branch name as API is the one that actually generates the object that gets used.

I'm going to leave this as-is for now to avoid CI churn, etc as we should yoink this placeholder out once #7075 merges

Copy link
Copy Markdown
Member

@Nishnha Nishnha left a comment

Choose a reason for hiding this comment

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

The tests are pretty comprehensive, thanks!

Had one minor nit around the name of the default dependency group, but the actual branch name changes LGTM

@brrygrdn brrygrdn force-pushed the brrygrdn/finalise-dependency-group-branch-naming branch from ca5705c to 0193c77 Compare April 14, 2023 16:39
@brrygrdn brrygrdn merged commit 84a53a3 into main Apr 14, 2023
@brrygrdn brrygrdn deleted the brrygrdn/finalise-dependency-group-branch-naming branch April 14, 2023 17:45
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