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

command: Add redirect support to 0.13upgrade #26061

Merged
merged 1 commit into from
Sep 1, 2020

Conversation

alisdair
Copy link
Contributor

@alisdair alisdair commented Aug 31, 2020

If a provider changes namespace in the registry, we can detect this when running the 0.13upgrade command. As long as there is a version matching the user's constraints, we now use the provider's new source address. Otherwise, warn the user that the provider has moved and a version upgrade is necessary to move to it.

Screenshots

Successful provider redirect, due to version constraint being met by the redirect target:

moved

Cannot follow the redirect, as the version constraint cannot be met:

warning

@alisdair alisdair requested review from justincampbell and a team August 31, 2020 15:53
@codecov
Copy link

codecov bot commented Aug 31, 2020

Codecov Report

Merging #26061 into master will decrease coverage by 0.01%.
The diff coverage is 48.54%.

Impacted Files Coverage Δ
command/013_config_upgrade.go 71.45% <41.93%> (-4.56%) ⬇️
internal/getproviders/registry_client.go 65.91% <45.00%> (-0.43%) ⬇️
internal/getproviders/registry_source.go 68.96% <50.00%> (ø)
command/init.go 66.66% <60.00%> (-0.17%) ⬇️
internal/getproviders/legacy_lookup.go 51.28% <78.57%> (+8.85%) ⬆️
dag/marshal.go 54.44% <0.00%> (+1.11%) ⬆️

tfdiags.Warning,
"Provider has moved",
fmt.Sprintf(
"Provider %q has moved to %q. You will need to upgrade to a new version before changing the source.",
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be unclear to users - do you mean upgrade terraform, or upgrade the pinned provider version? It's also not quite clear that this is optional. This is my awkward take at the message, it's not necessarily better, just want to see what you think:

"Provider %q has moved to %q. No action is required to continue using %q (provider) version %q (their constraint), but if you want to upgrade beyond version %s (last version available at the old location) you must also update the source:

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is a great suggestion!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great idea, thanks! Here's the warning I ended up with:

warning

Copy link
Contributor

@mildwonkey mildwonkey left a comment

Choose a reason for hiding this comment

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

This is a great update, thank you!

I left some nitpicky thoughts about the warning when the provider has moved but the config is pinned to the older (pre-move) version of the provider, but they are not meant to block merging - you can modify the message on your own, or not, and I still approve ✅

Copy link
Contributor

@justincampbell justincampbell 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 great to me from the Registry point of view. Thanks!

If a provider changes namespace in the registry, we can detect this when
running the 0.13upgrade command. As long as there is a version matching
the user's constraints, we now use the provider's new source address.
Otherwise, warn the user that the provider has moved and a version
upgrade is necessary to move to it.
@alisdair alisdair force-pushed the alisdair/013upgrade-registry-provider-redirect branch from dda6345 to fc7e467 Compare August 31, 2020 18:54
@alisdair alisdair merged commit 89e8d08 into master Sep 1, 2020
@alisdair alisdair deleted the alisdair/013upgrade-registry-provider-redirect branch September 1, 2020 12:59
@ghost
Copy link

ghost commented Oct 13, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked as resolved and limited conversation to collaborators Oct 13, 2020
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.

4 participants