Implement `plan -out` with cloud integration #33418

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 deployed to Preview June 23, 2023 00:21 View deployment

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

brandonc reviewed Jun 23, 2023

View reviewed changes

vercel bot deployed to Preview June 23, 2023 18:21 View deployment

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 deployed to Preview June 28, 2023 19:49 View deployment

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

vercel bot deployed to Preview June 29, 2023 17:15 View deployment

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 deployed to Preview June 29, 2023 18:02 View deployment

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 had a problem deploying to Preview July 1, 2023 00:12 Failure

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `plan -out` with cloud integration #33418

Implement `plan -out` with cloud integration #33418

nfagerlund commented Jun 23, 2023 •

edited

Loading

brandonc left a comment

brandonc Jun 23, 2023

nfagerlund Jun 23, 2023

Uk1288 left a comment

nfagerlund commented Jun 28, 2023

sebasslash left a comment

brandonc left a comment

brandonc Jun 30, 2023

nfagerlund Jul 1, 2023

brandonc Jun 30, 2023

sebasslash left a comment

sebasslash Jul 5, 2023

nfagerlund Jul 6, 2023

sebasslash left a comment •

edited

Loading

nfagerlund commented Jul 6, 2023

github-actions bot commented Jul 6, 2023

github-actions bot commented Dec 13, 2023

Implement plan -out with cloud integration #33418

Implement plan -out with cloud integration #33418

Conversation

nfagerlund commented Jun 23, 2023 • edited Loading

brandonc left a comment

Choose a reason for hiding this comment

brandonc Jun 23, 2023

Choose a reason for hiding this comment

nfagerlund Jun 23, 2023

Choose a reason for hiding this comment

Uk1288 left a comment

Choose a reason for hiding this comment

nfagerlund commented Jun 28, 2023

sebasslash left a comment

Choose a reason for hiding this comment

brandonc left a comment

Choose a reason for hiding this comment

brandonc Jun 30, 2023

Choose a reason for hiding this comment

nfagerlund Jul 1, 2023

Choose a reason for hiding this comment

brandonc Jun 30, 2023

Choose a reason for hiding this comment

sebasslash left a comment

Choose a reason for hiding this comment

sebasslash Jul 5, 2023

Choose a reason for hiding this comment

nfagerlund Jul 6, 2023

Choose a reason for hiding this comment

sebasslash left a comment • edited Loading

Choose a reason for hiding this comment

nfagerlund commented Jul 6, 2023

github-actions bot commented Jul 6, 2023

github-actions bot commented Dec 13, 2023

Implement `plan -out` with cloud integration #33418

Implement `plan -out` with cloud integration #33418

nfagerlund commented Jun 23, 2023 •

edited

Loading

sebasslash left a comment •

edited

Loading