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
command/providers: Show provider requirements tree #25190
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ import ( | |
"strings" | ||
"testing" | ||
|
||
"github.com/hashicorp/terraform/helper/copy" | ||
"github.com/mitchellh/cli" | ||
) | ||
|
||
|
@@ -75,14 +76,10 @@ func TestProviders_noConfigs(t *testing.T) { | |
} | ||
|
||
func TestProviders_modules(t *testing.T) { | ||
cwd, err := os.Getwd() | ||
if err != nil { | ||
t.Fatalf("err: %s", err) | ||
} | ||
if err := os.Chdir(testFixturePath("providers/modules")); err != nil { | ||
t.Fatalf("err: %s", err) | ||
} | ||
defer os.Chdir(cwd) | ||
td := tempDir(t) | ||
copy.CopyDir(testFixturePath("providers/modules"), td) | ||
defer os.RemoveAll(td) | ||
defer testChdir(t, td)() | ||
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. Fixing a bug which left dangling 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. 🎉 thank you! that had even been pointed out to me and i forgot 🤦 |
||
|
||
// first run init with mock provider sources to install the module | ||
initUi := new(cli.MockUi) | ||
|
@@ -120,7 +117,8 @@ func TestProviders_modules(t *testing.T) { | |
wantOutput := []string{ | ||
"provider[registry.terraform.io/hashicorp/foo] 1.0.*", // from required_providers | ||
"provider[registry.terraform.io/hashicorp/bar] 2.0.0", // from provider config | ||
"provider[registry.terraform.io/hashicorp/baz]", // implied by a resource in the child module | ||
"── module.kiddo", // tree node for child module | ||
"provider[registry.terraform.io/hashicorp/baz]", // implied by a resource in the child module | ||
} | ||
|
||
output := ui.OutputWriter.String() | ||
|
@@ -156,6 +154,7 @@ func TestProviders_state(t *testing.T) { | |
wantOutput := []string{ | ||
"provider[registry.terraform.io/hashicorp/foo] 1.0.*", // from required_providers | ||
"provider[registry.terraform.io/hashicorp/bar] 2.0.0", // from a provider config block | ||
"Providers required by state", // header for state providers | ||
"provider[registry.terraform.io/hashicorp/baz]", // from a resouce in state (only) | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,6 @@ provider "bar" { | |
version = "2.0.0" | ||
} | ||
|
||
module "child" { | ||
module "kiddo" { | ||
source = "./child" | ||
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. Renamed so that the module didn't match the source directory, so we can verify that the name is shown in the tree. 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 kiddo/kinder, excellent choices |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -77,6 +77,15 @@ type Config struct { | |
Version *version.Version | ||
} | ||
|
||
// ModuleRequirements represents the provider requirements for an individual | ||
// module, along with references to any child modules. This is used to | ||
// determine which modules require which providers. | ||
type ModuleRequirements struct { | ||
Module *Module | ||
Requirements getproviders.Requirements | ||
Children map[string]*ModuleRequirements | ||
} | ||
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. This type and the method which returns an instance of it below are intended to be used in an upcoming improvement to the 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. thank you for this extra context! |
||
|
||
// NewEmptyConfig constructs a single-node configuration tree with an empty | ||
// root module. This is generally a pretty useless thing to do, so most callers | ||
// should instead use BuildConfig. | ||
|
@@ -175,12 +184,45 @@ func (c *Config) DescendentForInstance(path addrs.ModuleInstance) *Config { | |
func (c *Config) ProviderRequirements() (getproviders.Requirements, hcl.Diagnostics) { | ||
reqs := make(getproviders.Requirements) | ||
diags := c.addProviderRequirements(reqs) | ||
|
||
for _, childConfig := range c.Children { | ||
moreDiags := childConfig.addProviderRequirements(reqs) | ||
diags = append(diags, moreDiags...) | ||
} | ||
|
||
return reqs, diags | ||
} | ||
|
||
// ProviderRequirementsByModule searches the full tree of modules under the | ||
// receiver for both explicit and implicit dependencies on providers, | ||
// constructing a tree where the requirements are broken out by module. | ||
// | ||
// If the returned diagnostics includes errors then the resulting Requirements | ||
// may be incomplete. | ||
func (c *Config) ProviderRequirementsByModule() (*ModuleRequirements, hcl.Diagnostics) { | ||
reqs := make(getproviders.Requirements) | ||
diags := c.addProviderRequirements(reqs) | ||
|
||
children := make(map[string]*ModuleRequirements) | ||
for name, child := range c.Children { | ||
childReqs, childDiags := child.ProviderRequirementsByModule() | ||
children[name] = childReqs | ||
diags = append(diags, childDiags...) | ||
} | ||
|
||
ret := &ModuleRequirements{ | ||
Module: c.Module, | ||
Requirements: reqs, | ||
Children: children, | ||
} | ||
|
||
return ret, diags | ||
} | ||
|
||
// addProviderRequirements is the main part of the ProviderRequirements | ||
// implementation, gradually mutating a shared requirements object to | ||
// eventually return. | ||
// eventually return. This function only adds requirements for the top-level | ||
// module. | ||
func (c *Config) addProviderRequirements(reqs getproviders.Requirements) hcl.Diagnostics { | ||
var diags hcl.Diagnostics | ||
|
||
|
@@ -235,13 +277,6 @@ func (c *Config) addProviderRequirements(reqs getproviders.Requirements) hcl.Dia | |
} | ||
} | ||
|
||
// ...and now we'll recursively visit all of the child modules to merge | ||
// in their requirements too. | ||
for _, childConfig := range c.Children { | ||
moreDiags := childConfig.addProviderRequirements(reqs) | ||
diags = append(diags, moreDiags...) | ||
} | ||
|
||
return 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.
Was there a specific reason or benefit to make this a method on the
ProvidersCommand
? I'm not opposed per se, but I don't see it using anything from theProvidersCommand
to explain the change - without this other commands could call this function.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.
Ah, no good reason. I'm happy to change it back if you prefer!
In prototyping this feature I added another function to this file, and since it was also very providers-command specific I named it
providersCommandWhatever
as well. Then that made me realize that if we want to signal that the purpose of the function is specific to this command, making it a method is probably clearer than a long name prefix, so I did that for both. Then I removed that function and forgot about this one.I think we have both patterns elsewhere in the command package: methods which don't reference the receiver, and package-scoped functions which are really only related to one struct. I slightly prefer the former but only at a 2/5 level.
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.
Nah, my approval stands 😄
At the absolute "worst" (ie, no problem at all, a totally normal and fine situation), we make a future change if we decide to use some of that functionality in another command (and even then the method would likely remain, and the other bits factored out).