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(gomod): Update source import paths on major upgrade #9144

Merged
merged 9 commits into from Mar 27, 2021

Conversation

56KBs
Copy link
Contributor

@56KBs 56KBs commented Mar 15, 2021

Changes:

Add gomodUpdateImportPaths postUpdateOption to update source code import paths when major updates occur.

Uses marwan-at-work/mod to complete the source code update

Context:

Closes #3348

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 unit tests, or
  • No new tests but ran on a real repository, or
  • Both unit tests + ran on a real repository

Add gomodUpdateImportPaths postUpdateOption to update source code import paths when major updates occur.
@56KBs
Copy link
Contributor Author

56KBs commented Mar 15, 2021

A point to discuss from this PR is the interaction & effective requirement for running the gomodTidy post update option alongside this newly introduced option.

An example PR using just gomodUpdateImportPaths postUpdateOption can be seen here - https://github.com/56KBs/renovate-bug-mrr/pull/2.

You can see both imports exist in the go.mod file, this is due to the ordering of events that occur. go get runs, adding the dependency and re-adding the old import, marwan-at-work/mod then runs, updating the import paths. Nothing is run to remove the un-needed imports, I can see two ways to resolve this:

  1. All users of gomodUpdateImportPaths must also enable gomodTidy to get the real value from it
  2. Following gomodUpdateImportPaths making changes, the go get runs again, which should, in theory, remove the unneeded import

The ordering implemented in this PR is the only order it can be completed in, marwan-at-work/mod cannot occur before the go get is issued as it will fail, see here for the error: https://github.com/56KBs/renovate-bug-mrr/pull/1

@rarkins
Copy link
Collaborator

rarkins commented Mar 15, 2021

I have considered whether to run goModTidy by default before, and we could consider it again.

Alternatively, running it implicitly if it's a major update, but then the behavior would be inconsistent between major and non-major updates.

We should also consider whether to make this PR's functionality default too, if we are generating incorrect PRs without it anyway.

Running go get twice seems ok too though.

@56KBs
Copy link
Contributor Author

56KBs commented Mar 16, 2021

Having tested the double go get functionality, it sadly doesn't work as I expected - This makes it more complicated to resolve, without the goModTidy being default behaviour, or implicitly run too.

I personally am not sure why you wouldn't run goModTidy by default, but perhaps there are some edge cases I've not considered.

@rarkins
Copy link
Collaborator

rarkins commented Mar 16, 2021

@56KBs let's start by running goModTidy for all major updates, as part of the PR you're preparing. It sounds like we'll probably switch to running it by default for non-major updates too, but for now we can limit it to major-only, which are currently "partly broken" anyway.

FYI the main reason for not running tidy all the time was because initially it wasn't part of go modules, and the modules creator thought people shouldn't focus on "cleaning up" their modules definitions too obsessively. So if it was optional for go mod, we kept it as optional for Renovate. But from my point of view, it makes sense to run.

@56KBs 56KBs marked this pull request as ready for review March 17, 2021 11:33
@56KBs
Copy link
Contributor Author

56KBs commented Mar 17, 2021

I've made goModTidy implicitly enabled on all major updates. For now however I've left gomodUpdateImportPaths as optional - I'm happy to remove this postUpdateOption and make it default behaviour on major updates if desired.

Copy link
Collaborator

@rarkins rarkins left a comment

Choose a reason for hiding this comment

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

This looks good to me, I think best to leave it behind a feature flag like
you've done until a few people have validated it in practice. Before approving, can you point to some example PRs where it's working?

@56KBs
Copy link
Contributor Author

56KBs commented Mar 22, 2021

Having ran it over some larger examples, it flagged up a bug when going from v0 -> v1 (It shouldn't update any source code) - I've pushed a fix for it and here are some example PRs:

  1. https://github.com/56KBs/renovate-bug-mrr/pull/4 - Using a fork of the reproduction repo from Go mod update from v1 to v2 results in duplicate import #9127
  2. https://github.com/56KBs/github-responder/pull/7 - Using a fork of the repository from Go modules: update source code after major updates #3348
    1. https://github.com/56KBs/github-responder/pull/8 - Shows a v0 to v1 upgrade

@rarkins rarkins enabled auto-merge (squash) March 25, 2021 10:13
@rarkins rarkins merged commit 49a3a8a into renovatebot:master Mar 27, 2021
@renovate-release
Copy link
Collaborator

🎉 This PR is included in version 24.96.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@56KBs 56KBs deleted the feat/gomod-major-import-updates branch March 29, 2021 08:04
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 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.

Go modules: update source code after major updates
3 participants