Target upstream terraform-plugin-sdk/v2 instead of the Pulumi fork - #3496
Draft
iwahbe wants to merge 1 commit into
Draft
Target upstream terraform-plugin-sdk/v2 instead of the Pulumi fork#3496iwahbe wants to merge 1 commit into
iwahbe wants to merge 1 commit into
Conversation
The bridge depended on github.com/pulumi/terraform-plugin-sdk/v2 via a
`replace` directive. The fork carried three patches over upstream:
1. `PlanResourceChangeExtra`, a variant of `PlanResourceChange` that
exposes the computed `*terraform.InstanceDiff` and lets the caller
transform that diff before it is applied to produce the planned
state.
2. `exports.go`, which re-exported `HCL2ValueFromConfigValue` from the
SDK's internal `hcl2shim` package.
3. A `RunProviderInternalValidation` gate that let `Provider.Validate`
skip the slow `InternalValidate` walk at runtime.
This change removes the fork and targets upstream v2.40.1 (the version
the fork was based on, so runtime behavior is unchanged) directly:
* `planResourceChange` reimplements the planning half of
`PlanResourceChange` using the public `Resource.SimpleDiff` API to
obtain the `InstanceDiff`, applying ignoreChanges to the diff before
turning it into the planned state. Branches the bridge never
exercises (provider deferral, resource identity, RequiresReplace)
are omitted.
* `linkname.go` recovers the SDK's unexported and internal-only state
shaping helpers via `//go:linkname`. `linkname_test.go` exercises
every linked symbol so an SDK upgrade that renames one fails as a
test rather than at runtime.
* `Provider.Validate` is replaced with a direct call to
`schema.InternalMap(...).Validate`, which runs the same schema
validation without `InternalValidate`. The
`RunProviderInternalValidation` toggle and its test are removed.
Dropping the fork replace also shifts the module graph to newer,
properly pruned dependency versions, which removes ~365 stale
transitive go.sum entries.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3496 +/- ##
=======================================
Coverage 70.20% 70.21%
=======================================
Files 350 351 +1
Lines 38524 38553 +29
=======================================
+ Hits 27047 27069 +22
- Misses 9616 9618 +2
- Partials 1861 1866 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Goal
Remove the
replace github.com/hashicorp/terraform-plugin-sdk/v2 => github.com/pulumi/terraform-plugin-sdk/v2directive and depend on the upstream SDK directly.This is a feasibility spike / proof of concept opened as a draft to confirm the approach builds and passes tests in CI.
What the fork provided
Diffing the fork against its upstream base (~v2.40.0), it carried exactly three patches:
PlanResourceChangeExtra— a variant ofPlanResourceChangethat surfaces the computed*terraform.InstanceDiff(upstream discards it) and adds aTransformInstanceDiffhook to mutate the diff before it becomes planned state. The bridge needs theInstanceDiffto drive its own diff, and uses the hook to applyignoreChanges.exports.go— re-exportedHCL2ValueFromConfigValue(andNormalizeObjectFromLegacySDK, unused by the bridge) from the SDK'sinternal/packages.RunProviderInternalValidation— gatedProvider.Validate's call to the slowInternalValidate, defaulting to skip it at runtime.How this PR replaces them
plan_resource_change.goreimplements the planning half ofPlanResourceChangeusing the publicResource.SimpleDiffto obtain theInstanceDiff, appliesignoreChangesto that diff, then turns it into planned state. Branches the bridge never exercises (provider deferral, resource identity,RequiresReplace) are intentionally omitted.linkname.gorecovers the SDK's unexported / internal-only state-shaping helpers (HCL2ValueFromConfigValue,HCL2ValueFromFlatmap,ValuesSDKEquivalent,normalizeNullValues,copyTimeoutValues,setWriteOnlyNullValues,validateConfigNulls) via//go:linkname.setWriteOnlyNullValuestakes an internal*configschema.Block, passed asunsafe.Pointer.Provider.Validatenow callsschema.InternalMap(p.tf.Schema).Validate(...)directly (InternalMapis a type alias for the unexportedschemaMap), running the same validation withoutInternalValidate. The toggle and its test are removed.A guard test (
linkname_test.go) exercises every linked symbol, so an SDK upgrade that renames one fails as a test rather than at runtime.Dropping the fork also shifts the module graph to newer, pruned dependency versions, removing ~365 stale transitive
go.sumentries (mostlycloud.google.com/go/*).Local validation
go build ./...,go vet ./...,golangci-lint— cleango mod tidyidempotent,go mod verifypassespkg/tfshim/sdk-v2/...— 186 passed (incl. linkname guard)pkg/internal/tests/cross-tests/...— 252 passed (Pulumi/TF diff parity)pkg/tfbridge/— passedTradeoffs to discuss
This relocates the fork's ~120-line plan logic into the bridge plus a
//go:linknamecoupling to SDK internals. It removes the fork-publishing workflow at the cost of linkname brittleness across SDK upgrades (guarded by the test) and a hand-maintained copy of the plan path. Whether this is a net win over rebasing the fork is the open question this draft is meant to surface.