Skip to content
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

terraform modules cmd human view #36062

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Maed223
Copy link
Collaborator

@Maed223 Maed223 commented Nov 19, 2024

Implements a human view for what was previously a machine readable view only command terraform modules -json. The human view shows the nested hierarchy/composition of modules in configuration rather than the flat list given by the json alternative, as well as additional info about the provided version constraints for the module. Documentation and the command's help output has been updated in accordance with this addition.

To accommodate this nested format, some reworking of resolver.go has been done to produce a manifest with a nested structure of records (whereas before a flat list was given).

Screenshot 2024-11-20 at 5 17 24 PM

Target Release

1.11.x

Draft CHANGELOG entry

NEW FEATURES

  • A human readable view for the terraform modules command.

@Maed223 Maed223 force-pushed the TF-19313/modules-cmd-human-view branch from 0778f7a to 2f7081c Compare November 19, 2024 15:43
@Maed223 Maed223 force-pushed the TF-19313/modules-cmd-human-view branch from 2f7081c to 272130e Compare November 19, 2024 16:07
@Maed223 Maed223 changed the title Tf 19313/modules cmd human view terraform modules cmd human view Nov 19, 2024
@Maed223 Maed223 marked this pull request as ready for review November 19, 2024 16:09
@Maed223 Maed223 requested review from a team as code owners November 19, 2024 16:09
Copy link
Contributor

The equivalence tests will be updated. Please verify the changes here.

Copy link
Contributor

The equivalence tests will be updated. Please verify the changes here.

rkoron007
rkoron007 previously approved these changes Nov 19, 2024
Copy link
Contributor

@rkoron007 rkoron007 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Added a small opportunistic suggestion

website/docs/cli/commands/modules.mdx Show resolved Hide resolved
Copy link
Contributor

The equivalence tests will be updated. Please verify the changes here.

Copy link
Contributor

@rkoron007 rkoron007 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved with some small tweaks for style!

The following example is a representation of the human readable output that `terraform modules` returns. The nested structure represents the
module composition/heirarchy within your configuration.

```
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be shell? Or something like it? We wanna make sure we get syntax highlighting if we can!

Suggested change
```
```shell-session


We will introduce new major versions only within the bounds of
[the Terraform 1.0 Compatibility Promises](/terraform/language/v1-compatibility-promises).

## Output Format

The following example is a representation of the human readable output that `terraform modules` returns. The nested structure represents the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like putting the "what" first before diving into the example, but let me know what you think!

Suggested change
The following example is a representation of the human readable output that `terraform modules` returns. The nested structure represents the
The `terraform modules` command returns a nested structure, representing module composition and hierarchy within your configuration.
The following example demonstrates what the `terraform modules` command returns without any additional flags:

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(make sure to remove the line below if you accept this suggestion!

@uturunku1 uturunku1 self-requested a review November 22, 2024 21:29
Copy link
Contributor

@uturunku1 uturunku1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job!

One inconsistency I was noticing between this cmd and other similar cmds that have have a json view is that when we pass the wrong argument for "-json", then we get different types of outputs. See picture:

Screenshot 2024-11-22 at 1 41 00 PM

I think the most accurate output from those 3 is the one for the "schema" cmd. The one for "show" is completely off, and the one for "modules" is slightly deceiving. Though the good thing is that it mentions that you can run "-help" and that is how the user can figure out that the proper argument is "-json".

But I was curious if you have a suggestion for a better error we could output instead "Too many command line arguments". This is not a blocker by the way. Aside from this question, I am happy with the output you have implemented, and I really like how the tree presents nested modules.

Key string
Source addrs.ModuleSource
Version *version.Version
VersionConstraints version.Constraints
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good addition!

Children: manifest.Records,
})

v.view.streams.Println(fmt.Sprintf("Modules declared by configuration:\n\n%s", printRoot.String()))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that this has one extra new line that the "providers" command. But that's okay. I kind of prefer two new lines than a single one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants