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

backend/remote: Fix version check when migrating #29793

Merged
merged 1 commit into from
Oct 25, 2021

Conversation

alisdair
Copy link
Member

When migrating state to an existing Terraform Cloud workspace using the remote backend, we check the remote version is compatible with the local one by default.

This commit fixes two bugs in this code:

  • If using the "name" strategy for the remote backend, the list of destination workspaces is empty. This resulted in no version checking of the remote workspace, and we fell back to the string equality check.
  • The user-specified CLI flag -ignore-remote-version was not being applied for the state migration version checking.

Fixes #29789. Targeting 1.0 backport.

When migrating state to an existing Terraform Cloud workspace using the
remote backend, we check the remote version is compatible with the local
one by default.

This commit fixes two bugs in this code:

- If using the "name" strategy for the remote backend, the list of
  destination workspaces is empty. This resulted in no version checking
  of the remote workspace, and we fell back to the string equality
  check.
- The user-specified CLI flag `-ignore-remote-version` was not being
  applied for the state migration version checking.
@alisdair alisdair added backend/remote 1.0-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged labels Oct 21, 2021
@alisdair alisdair requested review from chrisarcand and a team October 21, 2021 18:13
@alisdair alisdair self-assigned this Oct 21, 2021
Copy link
Member

@nfagerlund nfagerlund left a comment

Choose a reason for hiding this comment

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

I haven't built this and run it, but the code seems perfectly reasonable.

Reckon we ought to have a test for this?

@alisdair
Copy link
Member Author

Agreed, but I think the only useful way to test it would be an e2e test, which as far as I'm aware we don't have the machinery for (yet).

Copy link
Member

@chrisarcand chrisarcand left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏻 Good catch!

// If there are no specified destination workspaces, perform a remote
// backend version check with the default workspace.
if len(destinationWorkspaces) == 0 {
diags := m.remoteBackendVersionCheck(opts.Destination, backend.DefaultStateName)
Copy link
Member

Choose a reason for hiding this comment

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

It took me far too long to follow the chain to verify this does the right thing with the default workspace. 😅

@github-actions
Copy link

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 Nov 25, 2021
@xiehan xiehan deleted the alisdair/fix-remote-backend-migrate-version-check branch July 22, 2024 12:10
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
3 participants