Closed dixneuf19 closed 1 year ago
OK so if I understand correctly:
terraform show
command does not have a -json
flag.Seeing that Terraform 0.13 was released more than two years ago, I am fine with not supporting Terraform 0.12.
I'm somewhat worried by the complexity of the end-to-end test with these changes. Suddenly each test case results in two sub-tests, instead on only one. The terraform state mv
command execution logic is also non-trivial. I think it should be moved to a test helper. Perhaps this helper could reuse some of the command execution logic we have in our terraform
package.
I've iterated on your code a bit and come up with the following changes:
commands
/blocks
pair of test cases now each have their own sub-testterraform state mv
commands in a shell, which makes the code much simpler (no removing quotes, etc.)func TestE2E(t *testing.T) {
// ...
binPath := buildBinary(t)
for _, tc := range tt {
t.Run(tc.name, func(t *testing.T) {
for _, outputFormat := range []string{"blocks", "commands"} {
t.Run(outputFormat, func(t *testing.T) {
/*
Skip tests that serve as documentation of known limitations or
use features incompatible with the Terraform CLI's version.
*/
if tc.skip {
t.Skip(tc.skipReason)
}
if outputFormat == "blocks" {
tfVer, err := terraform.NewRunner(".").Version()
if err != nil {
t.Fatalf("failed to get terraform version: %v", err)
}
if tfVer.LessThan(semver.MustParse("1.1")) {
t.Skip("terraform moves output format is only supported in terraform 1.1 and above")
}
}
/*
Create a fresh environment for each test.
*/
setupWorkdir(t, tc.workdir)
workdir := filepath.Join(tc.workdir, "refactored-code")
args := append(tc.args, fmt.Sprintf("-output=%s", outputFormat))
/*
Run tfautomv to generate `moved` blocks or `terraform state mv` commands.
*/
tfautomvCmd := exec.Command(binPath, args...)
tfautomvCmd.Dir = workdir
var tfautomvStdout bytes.Buffer
var tfautomvCompleteOutput bytes.Buffer
tfautomvCmd.Stdout = io.MultiWriter(&tfautomvStdout, &tfautomvCompleteOutput, os.Stderr)
tfautomvCmd.Stderr = io.MultiWriter(&tfautomvCompleteOutput, os.Stderr)
if err := tfautomvCmd.Run(); err != nil {
t.Fatalf("running tfautomv: %v", err)
}
/*
Validate that tfautomv produced the expected output.
*/
outputStr := tfautomvCompleteOutput.String()
for _, s := range tc.wantOutputInclude {
if !strings.Contains(outputStr, s) {
t.Errorf("output should contain %q but does not", s)
}
}
for _, s := range tc.wantOutputExclude {
if strings.Contains(outputStr, s) {
t.Errorf("output should not contain %q but does", s)
}
}
/*
If using `terraform state mv` commands, run them.
*/
if outputFormat == "commands" {
cmd := exec.Command("/bin/sh")
cmd.Dir = workdir
cmd.Stdin = &tfautomvStdout
cmd.Stdout = os.Stderr
cmd.Stderr = os.Stderr
if err := cmd.Run(); err != nil {
t.Fatalf("running terraform state mv commands: %v", err)
}
}
/*
Count how many changes remain in Terraform's plan.
*/
plan, err := terraform.NewRunner(workdir).Plan()
if err != nil {
t.Fatalf("terraform plan (after addings moves): %v", err)
}
changes := plan.NumChanges()
if changes != tc.wantChanges {
t.Errorf("%d changes remaining, want %d", changes, tc.wantChanges)
}
})
}
})
}
}
I think this PR is starting to look really good! Thanks for doing the work!
I think we still need to make a few changes:
After that, I think this PR will be ready.
I am still a bit reluctant about the two additional levels of nesting we are introducing in the TestE2E
function. However I do not think that that prevents us from merging this in :)
You'll need to update your branch to include the required tests. These tests are currently on the repo's main
branch.
Nice I learned a few things such as the sub-tests or how to execute directly shell commands !
The e2e function is now very long and indented, but for the moment it is still somewhat clean/clear. Splitting it up for the sake of factorization might be premature optimization. Only time will tell.
For the hacky support of v0.12.x
, I agree that it may be not be very interesting to add a regexp juste for one old version that you should not use anymore (well I know it is still used in some projects, but it should be an incentive to upgrade).
I included your code into the commits, and rebase everything to have a clear history.
Please challenge my formulation for the documentation, it might not be idiomatic english.
I have fixed the last issues regarding documentation, should be ready for merge now!
I tried to address #15, by doing the following:
-output=blocks
or-output=commands
.gitignore
withtfstate.*.backup
files generated byterraform state mv
I probably introduced some code smells, please suggest some changes if necessary!
Since it is the Hacktoberfest season, could you include the
hacktoberfest-accepted
label in the PR ? Or even add thehacktoberfest
topic to the repo to indicate that you’re looking for Hacktoberfest PR/MRs!