Implement terraform show for cloud plan files
#33451
Conversation
nfagerlund
force-pushed
the
nf/jun23-show-a-plan
branch
from
2d5aae7
to
2edff96
Compare
There was a problem hiding this comment.
I'm seeing a slightly misleading/compound error when hitting the common failures associated with showing a remote plan:
$ ./terraform-showplan show localplan.json
╷
│ Error: Failed to read the given file as a state or plan file
│
│ State read error: Error reading localplan.json as a statefile: Unsupported state file format: The state file does not have a "version" attribute, which
│ is required to identify the format version.
│
│ Plan read error: couldn't read information for cloud run run-CpWyXmejtSAMLPbT; make sure you've run `terraform login` and that you have permission to
│ view the run
╵
| if !plan.CanApply() { | ||
| opts = append(opts, jsonformat.CanNotApply) | ||
| opts = append(opts, plans.NoChanges) |
There was a problem hiding this comment.
Just sanity checking here: the new name captures all the reasons why !plan.CanApply() ? The old name seems to fit this description better since plan.CanApply is a black box
There was a problem hiding this comment.
🤔 That's a good question — I was going from the perspective of the output we traditionally give, and from that view, "CanNotApply" isn't really the truth — you can totally apply an empty plan.
And the logic inside CanApply() comes down to "are there concrete changes? OR, is it refresh-only and there's refresh changes?"
To me, yes, that sounds like they're equivalent... and if they later change to Not be equivalent, well, it's all in the internal packages and we can rationalize distinctions as they arise.
|
@brandonc Yeah, I'm not sure how to make those compound errors nicer. 😵 Open to suggestions! I guess we could make two separate error types for each file kind and distinguish between
...and if one of the two files returned the first kind, we make that the error? falling back to the compound error if no one had any idea what it was?? |
There was a problem hiding this comment.
🎉 the show commands worked nicely!
I found the errors bulky for a couple cases as well. It might be helpful to make the errors more concise and specific if possible.
- When running the show command for a plan that is still queued, awaiting planning. The state error still shows up, my expectation was to see only the second error since it is the most accurate one.
- When running the show command without a valid token: I expected only the authorization error, but I got both
Error: Failed to read the given file as a state or plan file
State read error: Error reading my. tfplan as a statefile: Unsupported state file format: The state file does not have a "version" attribute, which is required to identify the format version.
Plan read error: Failed to read organization "business-org" at host ...: Encountered an unexpected error while reading the organization settings: unauthorized
One suggestion is to have a meta data to make mark the struct as state or plan file once the file has been unwrapped, so that functions know which file is being dealt with in order to display specific errors, and then use the compound error as the fallback.
…a plan We need to be able to exchange the facts represented by `jsonformat.PlanRendererOpt` across some package boundaries, but that type's package is implicated in too many dependency chains to be safe for that purpose. So, let's make a new one. The plans package seems safe to import from all the places that must emit or accept this info.
Identical semantics and behavior, but now the type is a peer of plans.Mode rather than belonging to the jsonformat package.
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 PlanJSON 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.
Created by just running the relevant terraform commands locally.
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.
This mimics a lack of admin permissions, resulting in a 404.
1. 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. 2. 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. 3. Fill in some missing expectations around HasChanges when retrieving a run + plan.
nfagerlund
force-pushed
the
nf/jun23-show-a-plan
branch
from
2edff96
to
b6a11fa
Compare
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.)
|
Yeah, y'all are right about those error messages. 😅 I've tidied them up a bit — the blast radius is slightly large, but it should only affect callers that deliberately try to inspect the error types.
|
| // PlanJSON is a wrapper struct that associates a pre-baked JSON plan with | ||
| // several pieces of metadata that can't be derived directly from the JSON | ||
| // contents and must instead be discovered from a tfe.Run or tfe.Plan. The | ||
| // wrapper is useful for moving data between the Cloud backend (which is the | ||
| // only thing able to fetch the JSON and determine values for the metadata) and | ||
| // the command.ShowCommand and views.Show interface (which need to have all of | ||
| // this information together). | ||
| type PlanJSON struct { |
There was a problem hiding this comment.
💅 What do you think about the term RemotePlan or something similar? I think there ought to be some level of distinction with a JSON Plan and JSON Plan + external metadata
internal/cloud/backend_show.go
Outdated
| header := "" | ||
| footer := "" |
There was a problem hiding this comment.
💅 What's the reasoning for making the initial assignment to an empty string? Given the function can exit prior to these values being read from, would it be worth only initializing when you need it?
|
Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch. |
|
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. |
The final piece of the puzzle for saved cloud plans: using
terraform showandterraform show -jsonto inspect a cloud plan file!Rendering a human plan from redacted JSON is already a solved problem, so the bulk of this PR is all about getting ahold of the information we need and transporting it across the architecture of the app to where it's needed. Effectively that means teaching the show command when to instantiate a Cloud backend, and teaching the backend how to do all the fetching and metadata inspection we need.
Note: This PR currently has a duplicate of the go-tfe v1.29.0 update commit from #33418, because I needed the new run status constant. I'll rebase it out of whichever PR gets merged second.
To smoke test this: You can just hand-craft a saved plan JSON file and point the
hostnameandrun_idfields at any run you care to sniff at! Alternately, you could use a build from #33418 to save a plan, if your TFC org has been added to the saved-cloud-plans feature flag.