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

Implements the graph command #257

Merged
merged 2 commits into from
Dec 14, 2021
Merged

Implements the graph command #257

merged 2 commits into from
Dec 14, 2021

Conversation

Chrisell
Copy link
Contributor

@Chrisell Chrisell commented Dec 6, 2021

Implements the graph command for Terraform. At the moment, the output of the command is stored as just a string, but could potentially be loaded into an object that implements the DOT format.

@hashicorp-cla
Copy link

hashicorp-cla commented Dec 6, 2021

CLA assistant check
All committers have signed the CLA.

@Chrisell
Copy link
Contributor Author

Chrisell commented Dec 8, 2021

@radeksimko I was wondering if you had any feedback on this. You previously helped a teammate of mine with a similar command addition to tfexec. Thanks!

Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

At the moment, the output of the command is stored as just a string, but could potentially be loaded into an object that implements the DOT format.

That sounds like a good idea worth considering prior to tfexec v1, I'll file an issue to track this. Outputting just DOT as string for now seems reasonable!

It is also worth noting that graph command generally isn't considered "protected worfklow" under the Terraform v1 compatibility guarantees, so in theory the output format is still subject to change.

The PR looks good overall, I just left you a couple of minor suggestions/comments in-line primarily about flags and version gating. Once these are addressed I'm happy to merge this.

tfexec/graph.go Outdated Show resolved Hide resolved
tfexec/graph.go Outdated Show resolved Hide resolved
tfexec/graph.go Outdated Show resolved Hide resolved
tfexec/graph.go Outdated Show resolved Hide resolved
t.Fatalf("error running Apply: %s", err)
}

_, err = tf.Graph(context.Background())
Copy link
Member

Choose a reason for hiding this comment

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

Is it worth testing the output here as well? I presume it shouldn't differ between TF versions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed minor differences in string value for certain resources in the graph output. Because of this i am validating against one known good string consistent in all outputs. I'm not sure if this satisfies your request or not.

Copy link
Member

Choose a reason for hiding this comment

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

I think this is much better than before, thank you!

For context, this library is becoming one of the ways we smoke test new Terraform CLI releases, so the subtle differences is actually something we may want to know about going forward. However for now this will do. 👌🏻

@radeksimko radeksimko merged commit 38e1272 into hashicorp:main Dec 14, 2021
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