fix(internal/provider): handle unknown values in tf_vars and provisioner_tags#341
fix(internal/provider): handle unknown values in tf_vars and provisioner_tags#341stirby wants to merge 3 commits into
Conversation
…ner_tags Change TerraformVariables and ProvisionerTags fields in TemplateVersion from Go native slices ([]Variable) to types.Set, matching the existing SetNestedAttribute schema definition. Go slices cannot represent unknown Terraform values during plan, causing Value Conversion Error when these fields receive dynamic values through modules or variable interpolation. Add varsFromSet helper to centralize Set-to-slice conversion with proper null/unknown handling at all four call sites. Closes #305
| // It computes directory hashes for each version and validates version constraints. | ||
| // Unlike the previous attribute-level plan modifier, this method only writes | ||
| // directory_hash values via SetAttribute, avoiding reconstruction of the entire | ||
| // versions list which would strip cty-level sensitivity marks from tf_vars. |
There was a problem hiding this comment.
This fix needs a regression test
| }, | ||
| }, | ||
| PlanModifiers: []planmodifier.List{ | ||
| NewVersionsPlanModifier(), |
There was a problem hiding this comment.
If we can't use the attribute-level plan modifier anymore, why are we keeping the code for it?
| // relying on ID.IsUnknown() set by the plan modifier, which required | ||
| // reconstructing the entire versions list and stripped cty sensitivity marks. | ||
| var lv LastVersionsByHash | ||
| lvBytes, pvDiag := req.Private.GetKey(ctx, LastVersionsKey) |
There was a problem hiding this comment.
It looks like we're doing a lot of the same work here in Update, as we are in PlanModifyList? Why doesn't update just use the planned value?
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
jatcod3r
force-pushed
the
fix/template-unknown-set-values
branch
4 times, most recently
from
9b59886 to
edd5827
Compare
When coderd_template has sensitive tf_vars and a deferred dependency (like time_static) that gets replaced during apply, Terraform re-plans the resource. The previous plan modifier used types.ListValueFrom() to write the entire versions list back, which reconstructed tftypes values and stripped Terraform core's cty-level sensitivity marks. This caused: 'Provider produced inconsistent final plan: inconsistent values for sensitive attribute' Changes: - Replace attribute-level PlanModifyList with resource-level ModifyPlan that only writes directory_hash via SetAttribute (proven safe for sensitivity preservation) - Move version reconciliation logic (new vs reuse) from plan modifier into Update, re-deriving from private state instead of ID.IsUnknown() - Keep reconcileVersionIDs() for unit tests but no longer call from plan modifier Fixes #305
jatcod3r
force-pushed
the
fix/template-unknown-set-values
branch
from
edd5827 to
ac4d097
Compare
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac4d097082
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // carries the correct cty value with any sensitivity marks from the | ||
| // config. reconcileVersionIDs only modifies name when config is null, | ||
| // so there's nothing new to write for user-set names. | ||
| if configVersions[i].Name.IsNull() { |
There was a problem hiding this comment.
When versions[i].name is set from an expression that is unknown during planning, reconcileVersionIDs() can still resolve a stable existing ID (and internally backfill the prior name), but ModifyPlan only writes name back when configVersions[i].Name.IsNull(). This leaves a plan with known id + unknown name, and Update() later calls UpdateTemplateVersion with newState.Versions[idx].Name.ValueString() (empty string for unknown), which can trigger unintended rename attempts or API errors for dynamic-name configurations. Previously, the list-level plan modifier rewrote the full versions list, so reconciled names were preserved.
Useful? React with 👍 / 👎.
jatcod3r
force-pushed
the
fix/template-unknown-set-values
branch
from
78b8465 to
ac4d097
Compare
4 participants
Problem
When
tf_varsorprovisioner_tagsreceive dynamic values through modules or variable interpolation, the provider fails during plan with:The root cause is that
TemplateVersion.TerraformVariablesandTemplateVersion.ProvisionerTagsuse Go native slices ([]Variable), which cannot represent unknown Terraform values during plan. The schema already correctly defines these asSetNestedAttribute, but the struct type did not match.Fix
Change both fields from
[]Variabletotypes.Set, aligning the struct with the schema. Add avarsFromSethelper to centralize Set-to-slice conversion with proper null/unknown handling at all four call sites (newVersion,setPrivateState,tfVariablesChanged,reconcileVersionIDs).No schema changes, no breaking changes to HCL syntax or state format.
Approach evaluation
Five approaches were evaluated:
types.Setdirect (no helpers)types.List+ schema changetypes.Maptypes.Set+ helper functionsTesting
TestReconcileVersionIDssubtests)TestUnknownTerraformVariablesHandlingvalidates unknown, null, populated, and change-detection scenariosTestAccTemplateResourceAGPL,TestAccTemplateResourceBackCompat)terraform planwith the exact config from the issue: old binary reproduces the error, fixed binary passesCloses #305