Always regen _massdriver_variables.tf on bundle build#218
Conversation
chrisghill
requested review from
Copilot and
coryodaniel
and removed request for
coryodaniel
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| // If _massdriver_variables.tf already exists, rename it so airlock won't read it | ||
| // during the scan. This allows variables declared there to be regenerated. | ||
| // The defer below restores the backup on any error, or removes it on success. | ||
| if _, statErr := os.Stat(massdriverVarsFile); statErr == nil { |
There was a problem hiding this comment.
massdriverVarsBackup is always the fixed path "_massdriver_variables.tf.bak". If that backup already exists (e.g., a prior run crashed after the rename), os.Rename may fail on Windows (and can overwrite on Unix). Consider removing/archiving any existing backup first or using a unique backup name (PID/timestamp) to avoid collisions and cross-platform behavior differences.
| if _, statErr := os.Stat(massdriverVarsFile); statErr == nil { | |
| if _, statErr := os.Stat(massdriverVarsFile); statErr == nil { | |
| // Ensure any existing backup does not cause cross-platform rename issues. | |
| if _, backupStatErr := os.Stat(massdriverVarsBackup); backupStatErr == nil { | |
| if removeErr := os.Remove(massdriverVarsBackup); removeErr != nil { | |
| return removeErr | |
| } | |
| } else if !errors.Is(backupStatErr, os.ErrNotExist) { | |
| return backupStatErr | |
| } |
| @@ -49,12 +53,54 @@ func TestOpentofuExportMassdriverInputs(t *testing.T) { | |||
| }, | |||
| }, | |||
| }, | |||
| want: `// Auto-generated variable declarations from massdriver.yaml | |||
| variable "bar" { | |||
| want: autoGeneratedHeader + `variable "bar" { | |||
| type = string | |||
| } | |||
| `, | |||
| }, | |||
There was a problem hiding this comment.
The test fixtures used here don't appear to match the scenarios the test names/expectations describe. For example, testdata/opentofu/same.tf currently declares only "foo", but this case's md schema includes both "foo" and "bar" and expects no _massdriver_variables.tf output. Conversely, missingopentofu expects bar to be generated, but its fixture currently declares both variables. This makes these cases fail or test the wrong behavior; consider using the new tfFile field to point each case at the intended fixture (or update the fixtures to match the names).
pkg/provisioners/opentofu_test.go
Outdated
| if tc.existingMassdriverVars != "" { | ||
| err = os.WriteFile(path.Join(testDir, "_massdriver_variables.tf"), []byte(tc.existingMassdriverVars), 0644) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error writing existing massdriver vars: %s", err) |
There was a problem hiding this comment.
In this failure path you're using %s to format an error value (which expects a string). Prefer %v for errors to avoid relying on error implementing String() and to match the other t.Fatalf calls updated in this file.
| t.Fatalf("unexpected error writing existing massdriver vars: %s", err) | |
| t.Fatalf("unexpected error writing existing massdriver vars: %v", err) |
| "github.com/massdriver-cloud/mass/pkg/provisioners" | ||
| ) | ||
|
|
||
| const autoGeneratedHeader = "// This file is auto-generated by massdriver from your massdriver.yaml file.\n// Any changes made directly to this file will be overwritten on the next build.\n// To opt a variable out of regeneration, move it to another file (e.g. variables.tf).\n" |
There was a problem hiding this comment.
autoGeneratedHeader is now duplicated across multiple tests (and the provisioner implementation embeds the same header string). This is easy to let drift and causes brittle string-matching assertions. Consider defining the header once in the provisioners package (const) and reusing it in both the implementation and tests.
This PR makes it so that
mass bundle build(andpublish) will always regenerate the_massdriver_variables.tffile. Previously it would only generate missing variables. This new approach is more intuitive for users, fits the general development cycle better, and still gives users the ability to opt-out of auto-generation by move variable declarations to a separate file.