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: Better in-house provider install errors #26066

Merged
merged 3 commits into from
Sep 1, 2020

Conversation

alisdair
Copy link
Contributor

@alisdair alisdair commented Aug 31, 2020

When init attempts to install a legacy provider required by state and fails, but another provider with the same type is successfully installed, this almost definitely means that the user is migrating an in-house provider. The solution here is to use the terraform state replace-provider subcommand.

This commit makes that next step clearer, by detecting this specific case.

Screenshots

Before:

before

After:

after

@alisdair alisdair requested a review from a team August 31, 2020 21:06
@alisdair alisdair self-assigned this Aug 31, 2020
@codecov
Copy link

codecov bot commented Aug 31, 2020

Codecov Report

Merging #26066 into master will decrease coverage by 0.00%.
The diff coverage is 58.82%.

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/format/diagnostic.go 45.17% <71.42%> (+0.09%) ⬆️
command/init.go 67.02% <72.97%> (+0.19%) ⬆️
internal/getproviders/legacy_lookup.go 51.28% <78.57%> (+8.85%) ⬆️
command/output.go 47.41% <100.00%> (+1.00%) ⬆️
terraform/evaluate.go 53.05% <0.00%> (-0.44%) ⬇️
dag/marshal.go 54.44% <0.00%> (+1.11%) ⬆️

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.

I love it, love everything about it. Definitely the right idea 🎉

@alisdair alisdair force-pushed the alisdair/init-legacy-providers-required-by-state branch from 205f81f to 6a8268e Compare September 1, 2020 14:20
@alisdair
Copy link
Contributor Author

alisdair commented Sep 1, 2020

Added tests, updated the format of the displayed state replace-provider commands (now on three lines, to try as hard as possible to avoid the automatic line wrapper), and updated the 0.13 upgrade guide to use the new error message.

command/init.go Outdated

fmt.Fprintf(&detail, "Found unresolvable legacy provider references in state. It looks like these refer to in-house providers. You can update the resources in state with the following %s:\n", command)
for legacy, replacement := range stateReplaceProviders {
fmt.Fprintf(&detail, "\n terraform state replace-provider \\\n %s \\\n %s", legacy, replacement)
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a feeling this backslash-escaping of newlines won't work in the Windows command interpreter 😖 . Although it'll be ugly to read, I think it'd be safer to put this all on one line so that everyone can just copy-paste it into whatever shell they are using. 😞

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh sorry I just noticed in your last comment that you did this to avoid the automatic line wrapping. 🙄

I thought we had a rule in the line wrapping that it would not try to wrap a line that starts with leading whitespace, but I guess I imagined it. With that said, I think it could be reasonable to add such an exception, if that would make this easier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the prompt! I hadn't considered trying to adjust how we format diagnostics to allow longer command lines. I made an attempt in 3547f9e and updated the other commits accordingly.

Diagnostic detail lines sometimes contain lines which include commands
suggested for the user to execute. By convention, these start with
leading whitespace to indicate that they are not prose.

This commit changes the diagnostic formatter to wrap each line of the
detail separately, and skips word wrapping for lines prefixed with
space. This prevents ugly and confusing wrapping of long command lines.
When init attempts to install a legacy provider required by state and
fails, but another provider with the same type is successfully
installed, this almost definitely means that the user is migrating an
in-house provider. The solution here is to use the `terraform state
replace-provider` subcommand.

This commit makes that next step clearer, by detecting this specific
case, and displaying a list of commands to fix the existing state
provider references.
The error diagnostic shown when legacy state contains resources from
in-house providers has changed, so update references to it in the 0.13
upgrade guide.
@alisdair alisdair force-pushed the alisdair/init-legacy-providers-required-by-state branch from 6a8268e to f795083 Compare September 1, 2020 18:02
@alisdair alisdair merged commit 45ac265 into master Sep 1, 2020
@alisdair alisdair deleted the alisdair/init-legacy-providers-required-by-state branch September 1, 2020 19:18
@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.

3 participants