generate config new algo - #1292
Closed
ansgarm wants to merge 11 commits into
Closed
Conversation
ansgarm
force-pushed
the
generate-config-new-algo
branch
from
March 23, 2026 09:47
7f7a8b0 to
902e90f
Compare
ansgarm
force-pushed
the
generate-config-new-algo
branch
from
March 23, 2026 10:03
3a592fa to
99ded31
Compare
Rename the new path-based validator methods so GenerateResourceConfig can resolve validator groups by kind without ambiguous interface matches, and extract default-value helpers into a dedicated fwserver module.
Surface unexpected GenerateResourceConfig transform errors in diagnostics so import config generation fails with actionable provider-facing messages instead of silently ignoring internal issues.
Add black-box coverage for validator groups, timeouts, and optional numeric zero handling so GenerateResourceConfig behavior stays constrained end to end. Fix number decoding in empty optional normalization so zero values are correctly nulled instead of raising internal errors.
Ignore expected transform-path lookups inside atomic attributes and blocks while still propagating real schema lookup failures. This keeps empty optional normalization lint-clean without regressing GenerateResourceConfig behavior.
ansgarm
commented
Mar 30, 2026
Comment on lines
-186
to
-190
| // stateToConfig returns a *tfsdk.Config with a copied value from a tfsdk.State. | ||
| func stateToConfig(state tfsdk.State) *tfsdk.Config { | ||
| return &tfsdk.Config{ | ||
| Raw: state.Raw.Copy(), | ||
| Schema: state.Schema, |
SBGoods
reviewed
Apr 6, 2026
Comment on lines
+67
to
+78
| // smarter algorithm steps: | ||
| // 1) Set top level properties named id and timeouts to null | ||
| idPath := tftypes.NewAttributePath().WithAttributeName("id") | ||
|
|
||
| // id | ||
| idAttribute, err := req.State.Schema.AttributeAtTerraformPath(ctx, idPath) | ||
| if err == nil && idAttribute.IsComputed() && idAttribute.IsOptional() { | ||
| config = nullValueAtPath(config, idPath) | ||
| } // else: no id attribute, ignoring | ||
|
|
||
| // timeouts | ||
| timeoutPath := tftypes.NewAttributePath().WithAttributeName("timeouts") |
Contributor
There was a problem hiding this comment.
I think it would be better if steps 1 and 2 were handled inside of the same tftypes.Transform func.
SBGoods
reviewed
Apr 6, 2026
Comment on lines
+354
to
+396
| // nulls the value at the given path in the value and returns the modified value. If the path does not exist, the original value is returned unmodified. | ||
| func nullValueAtPath(value tftypes.Value, path *tftypes.AttributePath) tftypes.Value { | ||
| currentValue, ok := valueAtPath(value, path) | ||
| if !ok { | ||
| return value | ||
| } | ||
|
|
||
| newValue, err := replaceValueAtPath(value, path, tftypes.NewValue(currentValue.Type(), nil)) | ||
|
|
||
| if err != nil { | ||
| return value | ||
| } | ||
|
|
||
| return newValue | ||
| } | ||
|
|
||
| func replaceValueAtPath(value tftypes.Value, path *tftypes.AttributePath, replaceWith tftypes.Value) (tftypes.Value, error) { | ||
| steps := path.Steps() | ||
|
|
||
| // Top-level attribute replacement needs special handling. During transform the | ||
| // root object is visited at the empty path, so replacing a single-step path | ||
| // means rebuilding that root object with one field swapped out. | ||
| if len(steps) == 1 { | ||
| if attributeName, ok := steps[0].(tftypes.AttributeName); ok { | ||
| var objectValue map[string]tftypes.Value | ||
| if err := value.As(&objectValue); err != nil { | ||
| return value, err | ||
| } | ||
|
|
||
| copiedObjectValue := maps.Clone(objectValue) | ||
| copiedObjectValue[string(attributeName)] = replaceWith | ||
|
|
||
| return tftypes.NewValue(value.Type(), copiedObjectValue), nil | ||
| } | ||
| } | ||
|
|
||
| return tftypes.Transform(value, func(p *tftypes.AttributePath, v tftypes.Value) (tftypes.Value, error) { | ||
| if p.Equal(path) { | ||
| return replaceWith, nil | ||
| } | ||
|
|
||
| return v, nil | ||
| }) |
Contributor
There was a problem hiding this comment.
I don't think these helper methods are necessary, tfsdk.State should have a SetAttribute() method that you can use to set an attribute value to null at a specific path.
SBGoods
reviewed
Apr 6, 2026
Contributor
There was a problem hiding this comment.
This file is very difficult to read/understand conceptually. There are a lot of tiny helper functions that require me to jump around in the code and breaks my reading flow. It might be better to inline the logic of a lot of these functions back into their calling functions.
SBGoods
reviewed
Apr 6, 2026
Contributor
There was a problem hiding this comment.
We should also have test coverage for the different nested block/attribute combinations (list, set, map nested attributes/blocks).
Member
Author
|
superseded by #1297 which we're manually fixing :) |
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
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.
What this does
This improves the quality of generated resource config.
It now:
idandtimeoutsConflictsWith,ExactlyOneOf, andAlsoRequiresFor the validator group handling to work, validators need to implement the three new interfaces that expose the paths they validate. The
terraform-plugin-framework-validatorspackage will be updated to support that, so updating that package will be one way to pick this up.Why
The old behavior was what Terraform Core can do on its own.
This change improves the generated config using schema information that the provider has, but Terraform Core does not. That gives us better output during import and makes the generated config closer to something a user can actually keep instead of having to clean up by hand.
Notes
I kept the normal Framework default behavior instead of writing defaults straight into generated config across the board.
The one special case is
ExactlyOneOf, where using a default can help generate config that makes sense.Rollback Plan
Changes to Security Controls
Are there any changes to security controls (access controls, encryption, logging) in this pull request? If so, explain.