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

feat: expose merge strategy for configuration when automerging #10627

Merged

Conversation

jbirch-atlassian
Copy link
Contributor

@jbirch-atlassian jbirch-atlassian commented Jun 27, 2021

Changes:

Expose the underlying merge strategy used by our platforms as a
configurable option for each repository to configure with the option
automergeStrategy. Additionally implement automergeStrategy for the
Bitbucket Cloud platform, as the only platform that doesn't currently
respect the remote repository's configured merge strategy.

Context:

closes #7184

Documentation (please check one with an [x])

  • I have updated the documentation, or
  • No documentation update is required

How I've tested my work (please tick one)

I have verified these changes via:

  • Code inspection only, or
  • Newly added/modified unit tests, or
  • No unit tests but ran on a real repository, or
  • Both unit tests + ran on a real repository

@jbirch-atlassian
Copy link
Contributor Author

jbirch-atlassian commented Jun 27, 2021

I have two areas of feedback here that I'm happy to either modify this Pull Request for, or submit in a follow-up Pull Request.

  1. From an internal ergonomics perspective, I think I'd like mergeStrategy to always be required on mergePr. Then each platform would need to explicitly say what it doesn't support, instead of just "failing to know that merge strategies are a thing".
  2. From a user ergonomics perspective, I think I'd like to extract the merge strategy into some form of exported function on each platform like isMergable(pr, strat) or something, so that it's possible to do a dry-run and be told "Renovate will do nothing, because platform X does not support strategy Y". At the moment, dry-run will emit what Renovate is instructed to do, and could possibly be told at runtime "It turns out platform X can't do Y".

@jbirch-atlassian
Copy link
Contributor Author

jbirch-atlassian commented Jun 27, 2021

image

The screenshot here, from bottom to top, shows:

  • A pull request where the repository default is merge commit, and automergeStrategy was set to auto. It resulted in a merge commit.
  • A pull request where the repository default is merge commit, and automergeStrategy was set to squash. It resulted in a squashed commit.
  • A pull request where the repository default is merge commit, and automergeStrategy was set to rebase (unsupported by Bitbucket). It resulted in no changes between 1630639 and 3ef5842.
  • A pull request where the repository default is squash, and automergeStrategy was set to auto. It resulted in a squashed commit.

Renovate commits looks like my commits because of how I ran Renovate.

Expose the underlying merge strategy used by our platforms as a
configurable option for each repository to configure with the option
`automergeStrategy`. Additionally implement `automergeStrategy` for the
Bitbucket Cloud platform, as the only platform that doesn't currently
respect the remote repository's configured merge strategy.
@jbirch-atlassian
Copy link
Contributor Author

jbirch-atlassian commented Jun 27, 2021

I was rude and force-pushed because I noticed an error immediately after pushing, before anyone had a chance to review. The error was an incomplete list of allowedValues in lib/config/definitions.ts.

@jbirch-atlassian
Copy link
Contributor Author

One final point of discussion: this PR is a rather in depth approach to the original ticket, with the goal of making the strategy changable on a per-renovate-configuration level, as per the discussion in #7184. However, the only strictly required change in order to resolve #7184 is just... Don't provide the optional merge_strategy parameter to the Bitbucket Cloud API.

If we don't have the appetite yet to take on exposing the merge strategy as a configuration item, I'm happy to put up those changes as a less ambitious PR instead.

Thanks again for your time and advice, y'all.

Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

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

More later

lib/platform/bitbucket/index.ts Outdated Show resolved Hide resolved
lib/platform/bitbucket/utils.ts Outdated Show resolved Hide resolved
lib/platform/bitbucket/utils.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@HonkingGoose HonkingGoose left a comment

Choose a reason for hiding this comment

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

Some initial changes, more to come later.

docs/usage/configuration-options.md Outdated Show resolved Hide resolved
lib/platform/bitbucket/index.md Outdated Show resolved Hide resolved
lib/platform/gitlab/index.md Outdated Show resolved Hide resolved
lib/platform/types.ts Outdated Show resolved Hide resolved
lib/platform/types.ts Show resolved Hide resolved
A follow-up commit will be added to jostle some new types around in the bitbucket platform module.

Co-authored-by: HonkingGoose <34918129+HonkingGoose@users.noreply.github.com>
Co-authored-by: Michael Kriese <michael.kriese@visualon.de>
lib/platform/bitbucket-server/index.md Outdated Show resolved Hide resolved
lib/platform/bitbucket/index.md Outdated Show resolved Hide resolved
docs/usage/configuration-options.md Show resolved Hide resolved
Co-authored-by: HonkingGoose <34918129+HonkingGoose@users.noreply.github.com>
Co-authored-by: Michael Kriese <michael.kriese@visualon.de>
consistent with the existing code structure conventions. Additionally
remove a superfluous markdown link footer that came together strangely
when accepted as a change in GitHub.
Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

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

Please open an issue for all platforms missing implementation too.

@jbirch-atlassian
Copy link
Contributor Author

@viceice: I'm not sure what's happening, but you're listed as "requesting changes" despite everything being resolved. I think it's coming from the changes in lib/platform/bitbucket/index.md being resolved with the suggestions in a comment, not with your suggestion.

Is everything above board, or am I just failing to GitHub today?

@viceice
Copy link
Member

viceice commented Jul 15, 2021

The request changes will resolve when I approve this pr.

Please open issues for the platforms which are missing this feature in this PR. After that I'll approve. 😉

Copy link
Collaborator

@HonkingGoose HonkingGoose left a comment

Choose a reason for hiding this comment

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

Some things I noticed just now.

One thing is for @viceice to apply if OK, the others you can do yourself. 😉

lib/config/definitions.ts Outdated Show resolved Hide resolved
lib/platform/azure/index.md Outdated Show resolved Hide resolved
lib/platform/gitea/index.md Outdated Show resolved Hide resolved
lib/platform/github/index.md Outdated Show resolved Hide resolved
lib/platform/gitlab/index.md Outdated Show resolved Hide resolved
@jbirch-atlassian
Copy link
Contributor Author

I guess it's just I didn't know how to GitHub! Thanks for the quick education, appreciate it.

#10867, #10868, #10869, and #10870 have been created for the remaining platforms.

jbirch-atlassian and others added 2 commits July 15, 2021 15:53
Co-authored-by: HonkingGoose <34918129+HonkingGoose@users.noreply.github.com>
One sentence per line. Drop the final `.` on lists.
Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

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

So the last small thing before we are haape to merge.

lib/config/definitions.ts Outdated Show resolved Hide resolved
Co-authored-by: Michael Kriese <michael.kriese@visualon.de>
Copy link
Collaborator

@HonkingGoose HonkingGoose left a comment

Choose a reason for hiding this comment

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

2 small things, looks good to me otherwise. 😉

lib/config/definitions.ts Outdated Show resolved Hide resolved
lib/platform/azure/index.md Show resolved Hide resolved
Co-authored-by: HonkingGoose <34918129+HonkingGoose@users.noreply.github.com>
HonkingGoose
HonkingGoose previously approved these changes Jul 19, 2021
@jbirch-atlassian
Copy link
Contributor Author

Hey y'all, I think this one should be good to go now. I'll swing back around in a week or so and poke folk again if there's no movement.

@rarkins rarkins merged commit 3096f34 into renovatebot:main Jul 29, 2021
@renovate-release
Copy link
Collaborator

🎉 This PR is included in version 25.60.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@jbirch-atlassian jbirch-atlassian deleted the feat/bitbucket-merge-strategies branch July 29, 2021 21:24
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make merge strategy and message configurable for Bitbucket
6 participants