Implement `plan -out` with cloud integration by nfagerlund · Pull Request #33418 · hashicorp/terraform

nfagerlund · 2023-06-23T00:03:43Z

This is it! We can use terraform plan -out my.tfplan in cloud mode to create an arbitrary number of saved plan runs, and then terraform apply my.tfplan one of the resulting plans to make it the workspace's next apply.

Because save-plan is a new attribute in the runs API, it requires an update to go-tfe — hashicorp/go-tfe#724

To smoke test this: Make sure your TFC org has been added to the saved-cloud-plans feature flag.

brandonc

So exciting to get rid of that error

internal/cloud/backend_plan.go

brandonc · 2023-06-23T15:15:39Z

internal/cloud/backend_plan.go

-	if op.PlanOutPath != "" {
-		diags = diags.Append(tfdiags.Sourceless(
-			tfdiags.Error,
-			"Saving a generated plan is currently not supported",


I love it when "currently" becomes the past 😆

Uk1288

Very nice! this worked smoothly for me!

One comment, I still receive the footer To perform exactly these actions, run the following command to apply... even when my plan errors out. See image below:

I was thinking the apply footer would not be displayed in this case. WDYT?

internal/cloud/backend_plan.go

nfagerlund · 2023-06-28T01:05:22Z

@Uk1288 That IS odd! I also expected it to not show that... But this relates to what @sebasslash brought up, too, so let's talk tomorrow and see if we actually want to skip saving the plan at all. (If so, I figure the way to suppress the message will come out of that.)

so we don't have to remember the format version number

This displays the footer that explains what happens if you saved a plan file (or neglected to). We never used to show it, because we didn't support -out.

This was wrong when err != nil and saveErr == nil.

On further consideration, this is a nonsensical thing to do, and it doesn't match the behavior of plain CLI! Additionally, skip next steps message on failure.

sebasslash

Code-wise this looks awesome to me 👍. I have not smoke tested yet so I won't directly approve.

One minor thing: would it be possible to unit test instances where the run errors but the plan should be saved? I imagine with the mock API that might not be trivial to create. Or perhaps this might be better suited for an integration test.

internal/cloud/backend_plan.go

brandonc

Just some minor feedback on return values

brandonc · 2023-06-30T22:05:31Z

internal/cloud/backend_plan.go

+	// If the run errored, exit before checking whether to save a plan file
+	run, err := b.plan(stopCtx, cancelCtx, op, w)
+	if err != nil {
+		return run, err


If we return an error, we should be make it clear that we are returning nil, err, I think.

Hmm, yeah, all right

brandonc · 2023-06-30T22:08:30Z

internal/cloud/backend_plan.go

+		op.View.PlanNextStep(op.PlanOutPath, "")
+	}
+
+	return run, err


Same suggestion here. I think we want to be clear about returning nil if we are also returning an error.

sebasslash

Smoke tested and most things are looking good. I've got some concerns (and are mostly on the TFC side of things):

What worked:

Specifying -out creates a JSON artifact representing the remote plan in my root dir
Specifying the created artifact in terraform apply indeed triggers a run
Can't re-apply a saved plan
Error is handled when specified plan file doesn't exist, and when the artifact is modified in some way with an incorrect value.

What did not work:

The state version was not processed correctly (no workspace resources/outputs showing). If I try to run a regular apply afterwards, I get some 'drift detected (delete)', and Terraform taints the resources in my config.
I might be wrong here, but I expected the run to show up on the UI upon running terraform plan -out. It only appeared once the plan was applied. This was quite confusing if the run is the first on a workspace.
I expected the plan file to be deleted once applied since it can no longer be used.

sebasslash · 2023-07-05T19:32:59Z

internal/cloud/backend_plan.go

+		if err != nil {
+			return nil, err
+		}


If we reach this block, run should not be nil. Wondering if there might be unintended consequences with not returning the run at this point.

I think it's okay — the caller really needs to assume that the primary value is NOT safe to mess with if they got an err, and that's kind of just a limitation of having exactly two return values for failure handling.

sebasslash

I've discussed with @nfagerlund internally and several of the TFC-related issues I ran into turned out to be issues with my local dev or already planned work.

On the matter of removing the remote plan artifact on your FS upon applying the saved plan, it is expected to match the existing OSS behavior which does not delete the plan binary.

nfagerlund · 2023-07-06T20:15:37Z

(For posterity: the issue with the plan not appearing in the runs list is a pure UI bug in TFC: some slapstick around the run list taking its ball and going home if there's no "current" run, which is the case if you've never done an eager run or fully confirmed a saved plan run. Addressed in the UI PR!)

github-actions · 2023-07-06T20:16:34Z

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

github-actions · 2023-12-13T02:09:29Z

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.

nfagerlund changed the title ~~Implement plan~~ Implement plan -out with cloud integration Jun 23, 2023

nfagerlund force-pushed the nf/jun23-save-a-plan branch from da67c2e to e2aae10 Compare June 23, 2023 00:18

vercel bot temporarily deployed to Preview June 23, 2023 00:21 Inactive

nfagerlund marked this pull request as ready for review June 23, 2023 00:27

brandonc reviewed Jun 23, 2023

View reviewed changes

vercel bot temporarily deployed to Preview June 23, 2023 18:21 Inactive

Uk1288 reviewed Jun 27, 2023

View reviewed changes

sebasslash reviewed Jun 27, 2023

View reviewed changes

internal/cloud/backend_plan.go Outdated Show resolved Hide resolved

vercel bot temporarily deployed to Preview June 28, 2023 19:49 Inactive

nfagerlund force-pushed the nf/jun23-save-a-plan branch from bf55cfe to 989a704 Compare June 29, 2023 17:10

vercel bot temporarily deployed to Preview June 29, 2023 17:15 Inactive

nfagerlund added 7 commits June 29, 2023 10:58

Upgrade to go-tfe v1.29.0 (for saved plan run support)

1e16a9e

Add NewSavedPlanBookmark function

11743d1

so we don't have to remember the format version number

Implement plan -out for cloud, maybe

5c56ca9

Call op.View.PlanNextStep for terraform plan in cloud mode

eef35b9

This displays the footer that explains what happens if you saved a plan file (or neglected to). We never used to show it, because we didn't support -out.

Rewrite TestCloud_planWithPath to expect success!

f67a5da

Fix wonky logic for combining plan errors and save errors

aad14b4

This was wrong when err != nil and saveErr == nil.

Don't save errored plans

fecb608

On further consideration, this is a nonsensical thing to do, and it doesn't match the behavior of plain CLI! Additionally, skip next steps message on failure.

nfagerlund force-pushed the nf/jun23-save-a-plan branch from 989a704 to fecb608 Compare June 29, 2023 17:59

vercel bot requested a deployment to Preview June 29, 2023 18:02 Abandoned

vercel bot temporarily deployed to Preview June 29, 2023 18:02 Inactive

nfagerlund mentioned this pull request Jun 30, 2023

Implement terraform show for cloud plan files #33451

Merged

sebasslash reviewed Jun 30, 2023

View reviewed changes

internal/cloud/backend_plan.go Outdated Show resolved Hide resolved

internal/cloud/backend_plan.go Outdated Show resolved Hide resolved

brandonc reviewed Jun 30, 2023

View reviewed changes

nfagerlund added 2 commits June 30, 2023 17:08

Cloud actually DOES support genconfig!

213064d

Clarify nils in returns: you get run, OR err

38858b9

vercel bot temporarily deployed to Preview July 1, 2023 00:12 Inactive

sebasslash requested changes Jul 5, 2023

View reviewed changes

sebasslash approved these changes Jul 6, 2023

View reviewed changes

nfagerlund merged commit dda00c8 into cli-team/saved-cloud-plans Jul 6, 2023

nfagerlund deleted the nf/jun23-save-a-plan branch July 6, 2023 20:16

nfagerlund mentioned this pull request Jul 7, 2023

Implement saved cloud plans #33492

Merged

github-actions bot locked as resolved and limited conversation to collaborators Dec 13, 2023

Conversation

nfagerlund commented Jun 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brandonc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

brandonc Jun 23, 2023

Choose a reason for hiding this comment

Uh oh!

nfagerlund Jun 23, 2023

Choose a reason for hiding this comment

Uh oh!

Uk1288 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nfagerlund commented Jun 28, 2023

Uh oh!

sebasslash left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

brandonc left a comment

Choose a reason for hiding this comment

Uh oh!

brandonc Jun 30, 2023

Choose a reason for hiding this comment

Uh oh!

nfagerlund Jul 1, 2023

Choose a reason for hiding this comment

Uh oh!

brandonc Jun 30, 2023

Choose a reason for hiding this comment

Uh oh!

sebasslash left a comment

Choose a reason for hiding this comment

Uh oh!

sebasslash Jul 5, 2023

Choose a reason for hiding this comment

Uh oh!

nfagerlund Jul 6, 2023

Choose a reason for hiding this comment

Uh oh!

sebasslash left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nfagerlund commented Jul 6, 2023

Uh oh!

github-actions bot commented Jul 6, 2023

Uh oh!

github-actions bot commented Dec 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nfagerlund commented Jun 23, 2023 •

edited

Loading

sebasslash left a comment •

edited

Loading