Merged
Conversation
radditude
reviewed
Jul 19, 2023
Member
radditude
left a comment
radditude
left a comment
There was a problem hiding this comment.
Nice work! I'd like to get that error message in the local backend fixed up before merge, but otherwise this looks great.
Having this sitting loose in `cloud` proved problematic because of circular import dependencies. Putting it in a sub-package under cloud frees us up to reference the type from places like `internal/backend`!
so we don't have to remember the format version number
It displays a run header with link to web UI, like starting a new plan does, then confirms the run and streams the apply logs. If you can't apply the run (it's from a different workspace, is in an unconfirmable state, etc. etc.), it displays an error instead. Notable points along the way: * Implement `WrappedPlanFile` sum type, and update planfile consumers to use it instead of a plain `planfile.Reader`. * Enable applying a saved cloud plan * Update TFC mocks — add org name to workspace, and minimal support for includes on MockRuns.ReadWithOptions.
- Don't save errored plans. - Call op.View.PlanNextStep for terraform plan in cloud mode (We never used to show this footer, because we didn't support -out.) - Create non-speculative config version if saving plan - Rewrite TestCloud_planWithPath to expect success!
This commit replaces the existing jsonformat.PlanRendererOpt type with a new type with identical semantics, located in the plans package. We needed to be able to exchange the facts represented by `jsonformat.PlanRendererOpt` across some package boundaries, but the jsonformat package is implicated in too many dependency chains to be safe for that purpose! So, we had to make a new one. The plans package seems safe to import from all the places that must emit or accept this info, and already contains plans.Mode, which is effectively a sibling of this type.
Since `terraform show -json` needs to get a raw hunk of json bytes and sling it right back out again, it's going to be more convenient if plain `show` can ALSO take in raw json. In order for that to happen, I need a function that basically acts like `client.Plans.ReadJSONOutput()`, without eagerly unmarshalling that `jsonformat.Plan` struct. As a slight bonus, this also lets us make the tfe client mocks slightly stupider.
To do the "human" version of showing a plan, we need more than just the redacted plan JSON itself; we also need a bunch of extra information that only the Cloud backend is in a position to find out (since it's the only one holding a configured go-tfe client instance). So, this method takes a run ID and hostname, finds out everything we're going to need, and returns it wrapped up in a RemotePlanJSON struct.
One funny bit: We need to know the ViewType at the point where we ask the Cloud backend for the plan JSON, because we need to switch between two distinctly different formats for human show vs. `show -json`. I chose to pass that by stashing it on the command struct; passing it as an argument would also work, but one, the argument lists in these nested method calls were getting a little unwieldy, and two, many of these functions had to be receiver methods anyway in order to call methods on Meta.
- Add plausible unredacted plan json for `plan-json-{basic,full}` testdata --
Created by just running the relevant terraform commands locally.
- Add plan-json-no-changes testdata --
The unredacted json was organically grown, but I edited the log and redacted
json by hand to match what I observed from a real but unrelated
planned-and-finished run in TFC.
- Add plan-json-basic-no-unredacted testdata --
This mimics a lack of admin permissions, resulting in a 404.
- Hook up `MockPlans.ReadJSONOutput` to test fixtures, when present.
This method has been implemented for ages, and has had a backing store for
unredacted plan json, but has been effectively a no-op since nothing ever
fills that backing store. So, when creating a mock plan, make an attempt to
read unredacted json and stow it in the mocks on success.
- Make it possible to get the entire MockClient for a test backend
In order to test some things, I'm going to need to mess with the internal
state of runs and plans beyond what the go-tfe client API allows. I could add
magic special-casing to the mock API methods, or I could locate the
shenanigans next to the test that actually exploits it. The latter seems more
comprehensible, but I need access to the full mock client struct in order to
mess with its interior.
- Fill in some missing expectations around HasChanges when retrieving a run +
plan.
This commit uses Go's error wrapping features to transparently add some optional info to certain planfile/state read errors. Specifically, we wrap errors when we think we've identified the file type but are somehow unable to use it. Callers that aren't interested in what we think about our input can just ignore the wrapping; callers that ARE interested can use `errors.As()`.
Since terraform show can accept three different kinds of file to act on, its error messages were starting to become untidy and unhelpful. The main issue was that if we successfully identified the file type but then ran into some problem while reading or processing it, the "real" error would be obscured by some other useless errors (since a file of one type is necessarily invalid as the other types). This commit tries to winnow it down to just one best error message, in the "happy path" case where we know what we're dealing with but hit a snag. (If we still have no idea, then we fall back to dumping everything.)
sebasslash
force-pushed
the
cli-team/saved-cloud-plans
branch
from
59c5d98 to
7cf27f5
Compare
sebasslash
force-pushed
the
cli-team/saved-cloud-plans
branch
from
7cf27f5 to
d4e061c
Compare
sebasslash
force-pushed
the
cli-team/saved-cloud-plans
branch
from
d4e061c to
e6c0e7b
Compare
radditude
approved these changes
Jul 24, 2023
sebasslash
force-pushed
the
cli-team/saved-cloud-plans
branch
from
e6c0e7b to
08e58fd
Compare
Contributor
|
Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch. |
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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hello! The CLI team has been working on Terraform Cloud support for:
terraform plan -out PLANFILEterraform show PLANFILE(with and without-json, which requires admin on workspace)terraform apply PLANFILEThis PR implements all of the above! Which is why it's a little on the chonky side.
Constituent prior PRs
We did this work as separate topic-based PRs against a WIP branch. Please refer to the following PRs for tons of additional context, and reduced scope for review! (Note that this final PR branch has been rebased and cleaned up a bit, so the commits might not correspond exactly, even though the main units of work do.)
terraform apply PLANplan -outwith cloud integration #33418 —terraform plan -out PLANterraform showfor cloud plan files #33451 —terraform show PLAN(andshow -json)The main upshot
saved-cloud-plansfeature flag, which is enabled in prod and oasis for orgs w/ hashicorp email addresses. This flag will go public to coincide with the 1.6 beta series.save-planrun attribute, specified on creation and included in reads.planned_and_savedrun status, and can be confirmed for apply (at which point they become the current run for their workspace, take the lock, and block other runs from applying until done. Like normal!). Once a new state version is created in the workspace, any existingplanned_and_savedruns become stale and automatically transition to thediscardedstatus.show, moving a little enum type to a different package so additional packages can use it, etc.)Target Release
1.6.0
Draft CHANGELOG entry
NEW FEATURES
terraform plan -out PLANFILE,terraform apply PLANFILE, andterraform show PLANFILEcommand variants when connected to a Terraform Cloud workspace.