command/providers: Show provider requirements tree #25190
Conversation
Providers can be required from multiple sources. The previous implementation of the providers sub-command displayed only a flat list of provider requirements, which made it difficult to see which modules required each provider. This commit reintroduces the tree display of provider requirements, and adds a separate output block for providers required by existing state.
Codecov Report
|
| td := tempDir(t) | ||
| copy.CopyDir(testFixturePath("providers/modules"), td) | ||
| defer os.RemoveAll(td) | ||
| defer testChdir(t, td)() |
There was a problem hiding this comment.
Fixing a bug which left dangling .terraform state when running tests.
There was a problem hiding this comment.
🎉 thank you! that had even been pointed out to me and i forgot 🤦
| @@ -10,6 +10,6 @@ provider "bar" { | |||
| version = "2.0.0" | |||
| } | |||
|
|
|||
| module "child" { | |||
| module "kiddo" { | |||
| source = "./child" | |||
There was a problem hiding this comment.
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.
love kiddo/kinder, excellent choices
| Module *Module | ||
| Requirements getproviders.Requirements | ||
| Children map[string]*ModuleRequirements | ||
| } |
There was a problem hiding this comment.
This type and the method which returns an instance of it below are intended to be used in an upcoming improvement to the init command's diagnostics when providers fail to install. That's why the Module member is here, and also why it's in the configs package instead of command.
There was a problem hiding this comment.
thank you for this extra context!
There was a problem hiding this comment.
This looks fantastic @alisdair! 🎉 This is going to be a huge help for users (and us!) ❤️
I left one question, but it's absolutely not meant to indicate you need to change anything - just curious, no objections.
| td := tempDir(t) | ||
| copy.CopyDir(testFixturePath("providers/modules"), td) | ||
| defer os.RemoveAll(td) | ||
| defer testChdir(t, td)() |
There was a problem hiding this comment.
🎉 thank you! that had even been pointed out to me and i forgot 🤦
| c.showDiagnostics(diags) | ||
| if diags.HasErrors() { | ||
| return 1 | ||
| } | ||
| return 0 | ||
| } | ||
|
|
||
| func providersCommandPopulateTreeNode(node treeprint.Tree, deps getproviders.Requirements) { | ||
| for fqn, dep := range deps { | ||
| func (c *ProvidersCommand) populateTreeNode(tree treeprint.Tree, node *configs.ModuleRequirements) { |
There was a problem hiding this comment.
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 the ProvidersCommand to explain the change - without this other commands could call this function.
There was a problem hiding this comment.
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.
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).
| @@ -10,6 +10,6 @@ provider "bar" { | |||
| version = "2.0.0" | |||
| } | |||
|
|
|||
| module "child" { | |||
| module "kiddo" { | |||
| source = "./child" | |||
There was a problem hiding this comment.
love kiddo/kinder, excellent choices
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
Providers can be required from multiple sources. The previous implementation of the providers sub-command displayed only a flat list of provider requirements, which made it difficult to see which modules required each provider.
This commit reintroduces the tree display of provider requirements, and adds a separate output block for providers required by existing state.
Screenshots
Configuration with nested modules:
Un-upgraded configuration with providers required by configuration and state: