Skip to content

backend/remote: Support -target on plan and apply#24834

Merged
apparentlymart merged 5 commits intomasterfrom
f-remote-target
May 19, 2020
Merged

backend/remote: Support -target on plan and apply#24834
apparentlymart merged 5 commits intomasterfrom
f-remote-target

Conversation

@apparentlymart
Copy link
Copy Markdown
Contributor

@apparentlymart apparentlymart commented May 1, 2020

This is the local-CLI portion of allowing -target to be used with remote operations. -target addresses given on local CLI are copied into the API request to create a run, which Terraform Cloud will then in turn pass back into -target options for the remote Terraform CLI process.

Old versions of Terraform Cloud/Enterprise that lack support for this new attribute would silently ignore the target-addrs attribute and run an untargeted plan, so this also includes some version-sniffing so we can continue to return an error for an older server. The design of go-tfe's versioning API combined with the mocking approach of the remote backend makes it annoyingly messy to unit test the case where -target isn't available on the server yet, so as a pragmatic compromise we elected to just test that part manually, with the rationale that the "not supported" situation will go away in the not-too-distant future as new versions of Terraform Cloud and Terraform Enterprise roll out, so that's not a long-term maintenance concern.


This also includes some unrelated updates to backend/remote to deal with some upstream breaking changes in the hashicorp/go-tfe library, which now requires use of the server's opaque workspace ID rather than the (organization-name, workspace-name) pair it expected before.

@apparentlymart apparentlymart self-assigned this May 1, 2020
@mikenomitch
Copy link
Copy Markdown
Contributor

Might be worth adding a different default message when using targetAddrs here -

Message: tfe.String("Queued manually using Terraform"),

@mikenomitch mikenomitch self-requested a review May 11, 2020 21:03
@hashicorp-cla
Copy link
Copy Markdown

hashicorp-cla commented May 13, 2020

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link
Copy Markdown

codecov bot commented May 13, 2020

Codecov Report

Merging #24834 into master will decrease coverage by 0.01%.
The diff coverage is 60.00%.

Impacted Files Coverage Δ
backend/remote/backend_apply.go 74.17% <23.07%> (-5.69%) ⬇️
backend/remote/backend_plan.go 73.80% <52.38%> (-3.24%) ⬇️
backend/remote/backend_context.go 61.19% <65.38%> (-2.23%) ⬇️
backend/remote/backend_mock.go 62.65% <75.00%> (+0.26%) ⬆️
backend/remote/backend_common.go 54.57% <100.00%> (+0.96%) ⬆️
backend/remote/testing.go 90.21% <100.00%> (+0.21%) ⬆️
terraform/evaluate.go 53.15% <0.00%> (-0.46%) ⬇️

apparentlymart and others added 3 commits May 15, 2020 15:58
This includes a new TargetAddrs field on both Run and RunCreateOptions
which we'll use to send resource addresses that were specified using
-target on the CLI command line when using the remote backend.

There were some unrelated upstream breaking changes compared to the last
version we had vendored, so this commit also includes some changes to the
backend/remote package to work with this new API, which now requires the
remote backend to be aware of the remote system's opaque workspace id.
Previously we did not allow -target to be used with the remote backend
because there was no way to send the targets to Terraform Cloud/Enterprise
via the API.

There is now an attribute in the request for creating a plan that allows
us to send target addresses, so we'll remove that restriction and copy
the given target addresses into the API request.
@radditude radditude self-requested a review May 15, 2020 23:19
}

if len(op.Targets) != 0 {
currentAPIVersion, err := version.NewVersion(b.client.RemoteAPIVersion())
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@radditude maybe we should include a comment here noting that RemoteAPIVersion returns an invalid version number (empty string) for versions before 2.3, to help a future reader understand why a parse error is treated the same as a version mismatch below...

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good idea! Added 👍

@apparentlymart apparentlymart marked this pull request as ready for review May 15, 2020 23:44
The remote server might choose to skip running cost estimation for a
targeted plan, in which case we'll show a note about it in the UI and then
move on, rather than returning an "invalid status" error.

This new status isn't yet available in the go-tfe library as a constant,
so for now we have the string directly in our switch statement. This is
a pragmatic way to expedite getting the "critical path" of this feature
in place without blocking on changes to ancillary codebases. A subsequent
commit should switch this over to tfe.CostEstimateSkippedDueToTargeting
once that's available in a go-tfe release.
b.CLI.Output(b.Colorize().Color("Waiting for cost estimate to complete..." + elapsed + "\n"))
}
continue
case "skipped_due_to_targeting": // TEMP: not available in the go-tfe library yet; will update this to be tfe.CostEstimateSkippedDueToTargeting once that's available.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We're planning to submit this tfe.CostEstimateSkippedDueToTargeting upstream to go-tfe asynchronously from getting this PR in. Will update this to use the named constant in a subsequent PR once it's included in a go-tfe release.

@ghost
Copy link
Copy Markdown

ghost commented Jun 19, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Jun 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants