Support workspaces for multi-state migrations #31
Conversation
|
@dhaven Thank you for working on this! I'll review this after studying how workspace works in Terraform core. Please wait. |
tfexec/terraform_workspace_show.go
Outdated
| ) | ||
|
|
||
| // tfWorkspaceRe is a pattern to parse outputs from terraform workspace show. | ||
| var tfWorkspaceRe = regexp.MustCompile(`^[a-zA-Z\d_-]{1,90}`) |
There was a problem hiding this comment.
I couldn't find any docs for a valid workspace name.
Could you tell me where this regular expression came from?
There was a problem hiding this comment.
I found it here : https://www.terraform.io/docs/cloud/workspaces/naming.html
Workspace names need to be 90 characters or less and can only include letters, numbers, -, and _.
Looking online that doesn't actually seem to be correct and the actual expectation is that the workspace name should be url-safe. (see: hashicorp/terraform#24564).
All things considered I don't think it is useful to implement this check as it is already handled by terraform so I have simply removed it.
There was a problem hiding this comment.
It's a common pitfall for workspace. The word workspace has two meanings in Terraform. That is, one is a workspace for Terraform Cloud and the other is a workspace for Terraform CLI. The doc you liked is for Terraform Cloud. It's actually not related to terraform workspace commands.
From the error message, I found the validation for terraform workspace new. It only requires a URL safe string.
https://github.com/hashicorp/terraform/blob/v1.0.3/internal/command/workspace_command.go#L42-L47
However, I agree that we don't need duplicated validation here by ourselves 👍
| @@ -90,18 +113,19 @@ func TestAccMultiStateMigratorApply(t *testing.T) { | |||
| ctx := context.Background() | |||
|
|
|||
| fromBackend := tfexec.GetTestAccBackendS3Config(t.Name() + "/fromDir") | |||
| fromWorkspace := "work1" | |||
There was a problem hiding this comment.
I want to confirm this change doesn't break default workspace behavior.
Can we parameterize the fromWorkspace and toWorkspace variables with the table-driven test?
There was a problem hiding this comment.
Or just copy and paste to a new test case such as TestAccMultiStateMigratorApplyWithWorkspace.
There was a problem hiding this comment.
I have followed your suggestion to use table-driven test. Please let me know what you think about the new test and if any changes are required.
|
Hi, @dhaven. Sorry for the late review. Although creating a local workspace directory I think your implementation is almost good. I left some minor comments. Please check. Thanks again! |
Address owner comments
| ok: true, | ||
| }, | ||
| { | ||
| desc: "with existing workspace", |
There was a problem hiding this comment.
not sure what you mean here because the second test case doesn't expect an error from the terraform command. I've added a third test case to test for an error returned by terraform workspace select
There was a problem hiding this comment.
My mistake. I guess when I wrote this comment, I probably misread the 2nd test case returns an error, but now I'm reading the 2nd test case with fresh eyes, apparently I was wrong. Sorry. Although, it's good to add the 3rd error case you suggested. Thanks!
| if changed != tc.toStateExpectChange { | ||
| t.Fatalf("expected change in toDir is %t but actual value is %t", tc.toStateExpectChange, changed) | ||
| } | ||
| }) |
| * [multi_state mv](#multi_state-mv) | ||
| * [License](#license) | ||
| <!--te--> | ||
|
|
|
@dhaven Shipped it in v0.2.6. Thank you for your great work 🎉 |
Implements the workspace feature request from #3
Main logic is in setupWorkDir() where I create the local workspace folder required before calling OverrideBackendToLocal()
Would love some feedback on this PR @minamijoyo