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

Fix remote backend multi workspace state migration #28864

Merged

Conversation

alisdair
Copy link
Contributor

@alisdair alisdair commented Jun 2, 2021

Two changes, both addressing the same underlying issue:

  • Fix faulty state migration version check when using multiple workspaces

    When migrating multiple local workspaces to a remote backend target using the prefix argument, we need to perform the version check against all existing workspaces returned by the Workspaces method. Failing to do so will result in a version check error.

  • Add -ignore-remote-version flag for init as a fallback

    When performing state migration to a remote backend target, Terraform may fail due to mismatched remote and local Terraform versions. Here we add the -ignore-remote-version flag to allow users to ignore this version check when necessary.

Unfortunately neither of these are easily unit tested. I've manually verified that this fixes #28858 when testing against my reproduction.

When migrating multiple local workspaces to a remote backend target
using the `prefix` argument, we need to perform the version check
against all existing workspaces returned by the `Workspaces` method.
Failing to do so will result in a version check error.
When performing state migration to a remote backend target, Terraform
may fail due to mismatched remote and local Terraform versions. Here we
add the `-ignore-remote-version` flag to allow users to ignore this
version check when necessary.
@alisdair alisdair added backend/remote 0.15-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged labels Jun 2, 2021
@alisdair alisdair requested a review from a team June 2, 2021 19:46
@alisdair alisdair self-assigned this Jun 2, 2021
Copy link
Member

@radditude radditude left a comment

Choose a reason for hiding this comment

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

My manual testing looks good! I'm going to list my test steps here just because this was a slightly convoluted repro and I want to confirm that I didn't miss anything.

Multiple workspaces

  • created local workspaces one and two
  • ran terraform apply for each so they had state
  • created TFC workspaces test-one and test-two with Terraform versions set to 0.13.7
  • added remote backend configuration that looked like:
backend "remote" {
	organization = "my-org"
	
	workspaces {
		prefix = "test-"
	}
}
  • ran terraform init and attempted to migrate state, which failed as expected due to the mismatched versions
  • ran terraform init -ignore-remote-version, which succeeded and state was successfully migrated to the two TFC workspaces
  • verified the happy path (that is, state migration with the prefix option works out of the box when the local and remote versions are compatible)

Single workspace

  • double-checked that there's no regression when migrating state using the name option to configure a single workspace

@@ -57,7 +57,7 @@ func (m *Meta) backendMigrateState(opts *backendMigrateOpts) error {
errMigrateLoadStates), opts.OneType, err)
}

_, err = opts.Two.Workspaces()
twoWorkspaces, err := opts.Two.Workspaces()
Copy link
Member

@radditude radditude Jun 4, 2021

Choose a reason for hiding this comment

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

A very small naming nit: I was momentarily confused by this variable name because my brain tried to parse two as the number of workspaces 😆 would it be weird to call this backendTwoWorkspaces instead? I realize that breaks with the naming convention of the other variables in this file, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that this variable naming is confusing, but as you noted the rest of the function (and the entire file) has this pattern established, and I don't want to change everything else so I intend to leave it as-is 😑

@alisdair alisdair added 1.0-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged and removed 0.15-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged labels Jun 8, 2021
@alisdair
Copy link
Contributor Author

alisdair commented Jun 8, 2021

Thank you for the very thorough manual testing, @radditude! 🙌

@alisdair alisdair merged commit 24ace6a into main Jun 8, 2021
@alisdair alisdair deleted the alisdair/fix-remote-backend-multi-workspace-state-migration branch June 8, 2021 14:13
@github-actions
Copy link

github-actions bot commented Jul 9, 2021

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
1.0-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged backend/remote
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error looking up workspace with multiple environments when prefix contains a digit/number
2 participants