Skip to content

test: fixes all tests with granular provisioner steps #21065

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.

Sign up for GitHub

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

Jump to bottom
Open
Emyrk wants to merge 3 commits into
base: stevenmasley/terraform_al_la_carte_feat
Choose a base branch
from
Open

test: fixes all tests with granular provisioner steps #21065

Emyrk wants to merge 3 commits into from
+1,291 −1,023

Conversation

@Emyrk
Copy link
Member

@Emyrk Emyrk commented Dec 2, 2025

No description provided.

@Emyrk Graphite App
Copy link
Member Author

Emyrk commented Dec 2, 2025 •
edited
Loading

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

@Emyrk Emyrk mentioned this pull request Dec 2, 2025
Open
@Emyrk Emyrk force-pushed the stevenmasley/terraform_al_la_carte_tests branch from b52ec3e to d3ae796 Compare December 2, 2025 18:23
@Emyrk Emyrk force-pushed the stevenmasley/terraform_al_la_carte_feat branch from 03de57d to 4a270f4 Compare December 2, 2025 18:23
@Emyrk Emyrk mentioned this pull request Dec 2, 2025
Merged
@Emyrk Emyrk changed the base branch from stevenmasley/terraform_al_la_carte_feat to graphite-base/21065 December 2, 2025 19:02
@Emyrk Emyrk mentioned this pull request Dec 2, 2025
Open
@Emyrk Emyrk force-pushed the graphite-base/21065 branch from 4a270f4 to 98ad5d4 Compare December 2, 2025 19:04
@Emyrk Emyrk force-pushed the stevenmasley/terraform_al_la_carte_tests branch from d3ae796 to f1bb408 Compare December 2, 2025 19:04
@graphite-app graphite-app bot changed the base branch from graphite-base/21065 to stevenmasley/terraform_al_la_carte_feat December 2, 2025 19:05
@Emyrk Emyrk force-pushed the stevenmasley/terraform_al_la_carte_tests branch from f1bb408 to 2b7bcfc Compare December 2, 2025 19:05
@Emyrk Emyrk changed the base branch from stevenmasley/terraform_al_la_carte_feat to graphite-base/21065 December 3, 2025 12:03
@Emyrk Emyrk force-pushed the graphite-base/21065 branch from 98ad5d4 to 3c9ea20 Compare December 9, 2025 15:23
@Emyrk Emyrk force-pushed the stevenmasley/terraform_al_la_carte_tests branch from 2b7bcfc to 64b9523 Compare December 9, 2025 15:23
@Emyrk Emyrk changed the base branch from graphite-base/21065 to stevenmasley/terraform_al_la_carte_feat December 9, 2025 15:24
@Emyrk Emyrk force-pushed the stevenmasley/terraform_al_la_carte_feat branch from 3c9ea20 to 3b5d651 Compare December 9, 2025 15:37
@Emyrk Emyrk force-pushed the stevenmasley/terraform_al_la_carte_tests branch from 64b9523 to b1ae149 Compare December 9, 2025 15:37
@Emyrk Emyrk force-pushed the stevenmasley/terraform_al_la_carte_feat branch from 3b5d651 to 04bc86a Compare December 9, 2025 15:51
@Emyrk Emyrk force-pushed the stevenmasley/terraform_al_la_carte_tests branch 5 times, most recently from 32cab2f to 6edb39c Compare December 9, 2025 17:44
@Emyrk Emyrk marked this pull request as ready for review December 9, 2025 17:53
@Emyrk Emyrk requested a review from spikecurtis December 9, 2025 17:54
@Emyrk Emyrk force-pushed the stevenmasley/terraform_al_la_carte_tests branch 3 times, most recently from da8b8e2 to 1ef7c09 Compare December 10, 2025 16:08
@Emyrk Emyrk requested a review from johnstcn as a code owner December 10, 2025 16:08
spikecurtis
spikecurtis reviewed Dec 11, 2025
View reviewed changes
Copy link
Contributor

@spikecurtis spikecurtis left a comment

Choose a reason for hiding this comment

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

At a high level, I find the echo.Responses initializations confusing now, because they often include only a subset of responses of commands that will be run.

There is a lot of spooky copying from one response type to another in TarWithOptions , like from apply to graph, but also graph to apply?, and many others.

I think that either the tests need to be explicit about the all the responses, or we need some new abstraction that allows you to ergonomically setup the echo (e.g. a builder).

provisioner/terraform/parse_test.go
}
}
}
`,
Copy link
Contributor

@spikecurtis spikecurtis Dec 11, 2025

Choose a reason for hiding this comment

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

Why is this required now when it wasn't before? Wasn't terraform init implicit in configuring the session? I don't get what's changed.

Copy link
Member Author

@Emyrk Emyrk Dec 11, 2025

Choose a reason for hiding this comment

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

No it was not. configure did not run terraform init. So parse happened before init.

Meaning you could have Parsed a template that would fail terraform init before (and it did fail).

Now Parse happens after init, which imo makes more sense to validate with the terraform cli, and not our hcl parser.

These tests however are affected, they are now heavier, running the terraform executable.

Copy link
Contributor

@spikecurtis spikecurtis Dec 12, 2025

Choose a reason for hiding this comment

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

I don't like running a real terraform init because it causes the test to download the terraform provider from the internet, which increases flakiness. We just had a load of tests fail like that last night.

If parse is expecting some init results now, we should fake them in these tests.

Copy link
Member Author

@Emyrk Emyrk Dec 12, 2025

Choose a reason for hiding this comment

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

Let me see what I can do

Copy link
Member Author

@Emyrk Emyrk Dec 12, 2025

Choose a reason for hiding this comment

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

@spikecurtis how about this solution?
6025829

We have a fake-terraform.sh that returns a static set of terraform logs. So we never run real terraform anymore for parse tests

@Emyrk Emyrk force-pushed the stevenmasley/terraform_al_la_carte_tests branch from 1ef7c09 to f06a77a Compare December 11, 2025 14:35
@Emyrk
Copy link
Member Author

Emyrk commented Dec 11, 2025

At a high level, I find the echo.Responses initializations confusing now, because they often include only a subset of responses of commands that will be run.
There is a lot of spooky copying from one response type to another in TarWithOptions , like from apply to graph, but also graph to apply?, and many others.

Yes the copying existed before to set sane defaults from what I could tell. If an Apply was given, then Plan was auto generated. The new paradigm means Graph is kinda the source of truth instead. The copy from Apply was to keep the previous behavior.

I think that either the tests need to be explicit about the all the responses, or we need some new abstraction that allows you to ergonomically setup the echo (e.g. a builder).

A builder would be nice, I never liked the current pattern as the types are so deeply nested. Some WithDefaults() could populate the responses that are omitted.

I will make a PR in this stack where I see how much work it is to explicitly set all responses across all these tests...

@Emyrk Emyrk mentioned this pull request Dec 11, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@spikecurtis spikecurtis spikecurtis left review comments

@johnstcn johnstcn Awaiting requested review from johnstcn johnstcn is a code owner

Assignees

@Emyrk Emyrk

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Emyrk @spikecurtis