Skip to content

command: remove 0.12upgrade#24403

Merged
mildwonkey merged 2 commits intomasterfrom
mildwonkey/remove-012-upgrade
Mar 19, 2020
Merged

command: remove 0.12upgrade#24403
mildwonkey merged 2 commits intomasterfrom
mildwonkey/remove-012-upgrade

Conversation

@mildwonkey
Copy link
Copy Markdown
Contributor

This PRs removes 012upgrade and the related upgrade code. I've left in the basic logic which detects when earlyconfig succeeds and regular config loading fails, which (stil)l suggests that terraform is dealing with 0.11-style syntax, but turned the warning into an error and added a message that suggests the user upgrade their syntax manually or use terraform 0.12 to run terraform init.

@ghost ghost added the sdkv1 [PRs only] Marks changes that may potentially need to be ported to the plugi nSDK label Mar 18, 2020
@mildwonkey mildwonkey requested a review from a team March 18, 2020 15:40
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 18, 2020

Codecov Report

Merging #24403 into master will decrease coverage by 0.38%.
The diff coverage is 85.18%.

Impacted Files Coverage Δ
commands.go 1.07% <0.00%> (ø)
command/init.go 69.01% <86.36%> (-0.95%) ⬇️
command/012_config_upgrade.go 100.00% <100.00%> (+100.00%) ⬆️
backend/remote/backend_common.go 54.92% <0.00%> (-0.71%) ⬇️
terraform/evaluate.go 41.26% <0.00%> (-0.66%) ⬇️
dag/walk.go 93.06% <0.00%> (-0.58%) ⬇️

Copy link
Copy Markdown
Contributor

@alisdair alisdair left a comment

Choose a reason for hiding this comment

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

👍 with one theoretical concern about exiting without displaying diags. Yay for code deletion!

command/init.go Outdated
// Errors from the early loader are generally not as high-quality since
// it has less context to work with.
c.Ui.Error(strings.TrimSpace(errInitConfigError))
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If I'm reading this correctly, if the early loader has errors and the real loader doesn't, we'll print an error but no diagnostics and won't exit. This seems like a basically-impossible situation to be in, but maybe we should try to handle it somehow anyway? We did before, I think.

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.

This prints the error, but waits till the next step to return so we can customize the response based on whether both config loaders fail, or just the latter.

Having said that, I'm making a big assumption that the config loader will always produce an error if the earlyconfig produces an error, and just because that's how it's supposed to work doesn't mean that's how it will work.

I'm going to make a couple of small changes and re-request review, thanks for bringing this up!

@mildwonkey
Copy link
Copy Markdown
Contributor Author

@alisdair: I rolled back my changes to the logic flow in init, thank you again, and plopped 0.12upgrade back with a warning message (I'm very iffy on the phrasing, but if we don't have a better idea immediately we can always fix it later).

@mildwonkey mildwonkey requested a review from alisdair March 18, 2020 19:48
@mildwonkey mildwonkey merged commit 5f313a6 into master Mar 19, 2020
@mildwonkey mildwonkey deleted the mildwonkey/remove-012-upgrade branch March 19, 2020 12:01
@ghost
Copy link
Copy Markdown

ghost commented Apr 19, 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 and limited conversation to collaborators Apr 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

sdkv1 [PRs only] Marks changes that may potentially need to be ported to the plugi nSDK

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants