-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
allow input validation with variable in source or version #5041
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
Changes from 3 commits
9d7f9b1
f1c46c1
6017839
30b97f4
0a411af
bce296b
3a439ea
4f66e86
e9a8894
fa7d98c
d626340
26a0a14
93e08d9
82301a5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was able to make an even more minimal example than the one in #4986
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Love the names on these fixtures. Reading the name of the fixture should tell you what the fixture is for. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| locals { | ||
| variable_source = "github.com/foo/bar" | ||
| } | ||
|
|
||
| module "module" { | ||
| source = local.variable_source | ||
| version = "0.0.0" | ||
| } |
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know empty files seem silly, but in the spirit of "minimal reproduction" I don't think you can get more minimal than empty!
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nothing wrong with it! If you want to be explicit about this, you can leave a one-line comment in the file that it's intentionally empty. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| locals { | ||
| variable_version = "0.0.0" | ||
| } | ||
|
|
||
| module "module" { | ||
| source = "github.com/foo/bar" | ||
| version = local.variable_version | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -614,6 +614,47 @@ func TestTerragruntExcludesFile(t *testing.T) { | |
| } | ||
| } | ||
|
|
||
| func TestHclvalidateValidConfig(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| t.Run("using --all", func(t *testing.T) { | ||
| t.Parallel() | ||
| helpers.CleanupTerraformFolder(t, testFixtureHclvalidate) | ||
| tmpEnvPath := helpers.CopyEnvironment(t, testFixtureHclvalidate) | ||
| rootPath := util.JoinPath(tmpEnvPath, testFixtureHclvalidate) | ||
|
|
||
| _, _, err := helpers.RunTerragruntCommandWithOutput(t, "terragrunt hcl validate --all --strict --inputs --working-dir "+path.Join(rootPath, "valid")) | ||
| require.NoError(t, err) | ||
| }) | ||
|
Comment on lines
620
to
628
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added this test, but actually it was already passing! Is
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's entirely possible there's a bug in the error handling. I think it should be returning an error if anything fails during an |
||
|
|
||
| t.Run("each validate individually", func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| helpers.CleanupTerraformFolder(t, testFixtureHclvalidate) | ||
| tmpEnvPath := helpers.CopyEnvironment(t, testFixtureHclvalidate) | ||
| rootPath := util.JoinPath(tmpEnvPath, testFixtureHclvalidate, "valid") | ||
|
|
||
| // Test each subdirectory individually | ||
| entries, err := os.ReadDir(rootPath) | ||
| require.NoError(t, err) | ||
|
|
||
| for _, entry := range entries { | ||
| if !entry.IsDir() { | ||
| continue | ||
| } | ||
|
|
||
| subPath := filepath.Join(rootPath, entry.Name()) | ||
|
|
||
| t.Run(entry.Name(), func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| _, _, err := helpers.RunTerragruntCommandWithOutput(t, "terragrunt hcl validate --strict --inputs --working-dir "+subPath) | ||
| require.NoError(t, err) | ||
| }) | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| func TestHclvalidateDiagnostic(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,8 @@ | ||
| package tf | ||
|
|
||
| import ( | ||
| "slices" | ||
|
|
||
| "github.com/gruntwork-io/terragrunt/internal/errors" | ||
| "github.com/hashicorp/terraform-config-inspect/tfconfig" | ||
| ) | ||
|
|
@@ -127,10 +129,35 @@ var ( | |
| } | ||
| ) | ||
|
|
||
| // diagnosticDoesNotAffectModuleVariables tells you if a diagnostic can be ignored for the purpose of extracting | ||
| // variables defined in a module. | ||
| func diagnosticDoesNotAffectModuleVariables(d tfconfig.Diagnostic) bool { | ||
| ignorableErrors := []tfconfig.Diagnostic{ | ||
| // These two occur when a module block uses a variable in the `version` or `source` fields. | ||
| // Terraform doesn't support this, but OpenTofu does. Either way our ability to extract variables is unaffected. | ||
| // | ||
| // What we really need is an OpenTofu version of terraform-config-inspect. This may work for now but as / if | ||
| // syntax continues to diverge we may run into other issues. | ||
| {Summary: "Variables not allowed", Detail: "Variables may not be used here."}, | ||
| {Summary: "Unsuitable value type", Detail: "Unsuitable value: value must be known"}, | ||
| } | ||
|
||
| if d.Severity != tfconfig.DiagError { | ||
| return true | ||
| } | ||
|
|
||
| i := slices.IndexFunc(ignorableErrors, func(ie tfconfig.Diagnostic) bool { | ||
|
||
| return ie.Summary == d.Summary && ie.Detail == d.Detail | ||
| }) | ||
|
|
||
| return i != -1 | ||
| } | ||
|
|
||
| // ModuleVariables will return all the variables defined in the downloaded terraform modules, taking into | ||
| // account all the generated sources. This function will return the required and optional variables separately. | ||
| func ModuleVariables(modulePath string) ([]string, []string, error) { | ||
| module, diags := tfconfig.LoadModule(modulePath) | ||
|
|
||
| diags = slices.DeleteFunc(diags, diagnosticDoesNotAffectModuleVariables) | ||
| if diags.HasErrors() { | ||
| return nil, nil, errors.New(diags) | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Open to input on this, or even removing it. But I definitely don't think the current wording is correct (since we're checking that the unit sets variables consumed by the module, the module doesn't set them).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would better wording be: "When enabled, Terragrunt will validate that all inputs set by Terragrunt units are valid OpenTofu/Terraform variables consumed by their respective modules"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel that only is accurate when using
--strict, without--strictyou can set inputs that are not consumed / don't exist as variables