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

Skip graph cycle validation when using the graph command #36186

Merged
merged 5 commits into from
Dec 11, 2024

Conversation

dsa0x
Copy link
Contributor

@dsa0x dsa0x commented Dec 10, 2024

The graph command should be able to generate graphs with cycles. However, this is only accepted when the -draw-cycles option is given. In this case, the graph validation would be skipped.

Fixes #34639

Target Release

1.11.x

Draft CHANGELOG entry

BUG FIXES

  • terraform graph -draw-cycles: Fix terraform graph command, and indicate the cycles.

@dsa0x dsa0x force-pushed the sams/skip-graph-cycle-validation branch from e295a9a to 0320f62 Compare December 10, 2024 12:42
Copy link
Contributor

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

if b.SkipGraphValidation {
return g, diags
}

if err := g.Validate(); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

This isn't the only Validate call, it needs to be called earlier too (this actually should never fail right now). You can't run TransitiveReduction if there are cycles, so we call Validate at least once when building the graph.

Copy link
Contributor Author

@dsa0x dsa0x Dec 11, 2024

Choose a reason for hiding this comment

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

There are 3 Validate calls. This one, the one in TransitiveReductionTransfomer, and one that is called on a subgraph during the graph walk.

This validation skip is only about building the graph, which is already done before a walk, so I think the latter matters less here. (maybe it would help for it to be called SkipGraphBuildValidation
Luckily, the one in the transitive reduction transformer does not return an error, it is simply a gate to prevent running TransitiveReduction on a cyclic graph.

However, I could try a different approach which would put the skipping step in the Validate method itself, so that every point where it is called gets that behaviour

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you think of this other approach? @jbardin
In 4675020, I moved the skip step to the Validate method, therefore it would not return an error if we are asked to skip validation. However, because the TransitiveReduction should not run if there are validation errors, we have to do an additional early return in the transformer.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I forgot that the transformer call drops the error! In that case yes, it is probably easiest to just return early here. Any errors that arise with the subgraphs are entirelyTerraform's problem, so not something a user would want to try debug with the graph.

@dsa0x dsa0x force-pushed the sams/skip-graph-cycle-validation branch from 55a2e68 to 7f44609 Compare December 11, 2024 15:45
@dsa0x dsa0x marked this pull request as ready for review December 11, 2024 15:49
@dsa0x dsa0x requested a review from a team as a code owner December 11, 2024 15:49
@dsa0x dsa0x merged commit aa38305 into main Dec 11, 2024
12 checks passed
@dsa0x dsa0x deleted the sams/skip-graph-cycle-validation branch December 11, 2024 16:32
Copy link
Contributor

Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch.

Copy link
Contributor

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 11, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

terraform graph -draw-cycles errors instead of showing cycles
3 participants