Only require any input parameters to be specified if they differ from the defaults #1558
Conversation
There was a problem hiding this comment.
Pull request overview
This PR enhances the Fleet integration policy resource to intelligently handle integration defaults, allowing users to specify only parameters that differ from defaults rather than requiring full configuration. The implementation adds a defaults attribute to track integration-specific default values and implements semantic equality comparison that considers these defaults when determining configuration drift.
Key Changes:
- Implemented default value tracking and application through a new
defaultscomputed attribute - Enhanced semantic equality logic to compare user configuration against defaults before detecting changes
- Removed requirement to specify all inputs/streams in configuration
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/fleet/integration_policy/update.go | Fetches package info to populate defaults during update operations |
| internal/fleet/integration_policy/testdata/integration_kafka.json | Adds Kafka integration test data for testing default value handling |
| internal/fleet/integration_policy/testdata/.../update_logfile_tags_only/integration_policy.tf | Test case for partial configuration specifying only non-default values |
| internal/fleet/integration_policy/testdata/.../unset/integration_policy.tf | Test case for minimal configuration without explicit inputs |
| internal/fleet/integration_policy/testdata/.../minimal/integration_policy.tf | Test case for minimal configuration with only essential inputs |
| internal/fleet/integration_policy/schema_v1.go | Sets defaults to null during v1 to v2 migration |
| internal/fleet/integration_policy/schema.go | Adds defaults attribute and plan modifiers for state management |
| internal/fleet/integration_policy/resource-description.md | Updates documentation to remove recommendation to specify all inputs |
| internal/fleet/integration_policy/read.go | Fetches package info during read operations |
| internal/fleet/integration_policy/models_test.go | Updates test to pass nil package info |
| internal/fleet/integration_policy/models_defaults_test.go | Comprehensive tests for default value extraction and application |
| internal/fleet/integration_policy/models_defaults.go | Core logic for extracting defaults from package metadata |
| internal/fleet/integration_policy/models.go | Integrates defaults into model population and API conversion |
| internal/fleet/integration_policy/inputs_value_test.go | Updates tests to handle defaults in equality checks |
| internal/fleet/integration_policy/inputs_value.go | Implements semantic equality using defaults for inputs |
| internal/fleet/integration_policy/inputs_type.go | Updates type signature to use InputType |
| internal/fleet/integration_policy/input_value_test.go | Tests for input-level semantic equality with defaults |
| internal/fleet/integration_policy/input_value.go | Implements semantic equality logic for individual inputs |
| internal/fleet/integration_policy/input_type.go | Defines custom type supporting semantic equality for inputs |
| internal/fleet/integration_policy/create.go | Fetches package info during create operations |
| internal/fleet/integration_policy/acc_test.go | Adds acceptance tests for partial and minimal configurations |
| docs/resources/fleet_integration_policy.md | Updates resource documentation to reflect new behavior |
| }, | ||
| "streams": schema.MapNestedAttribute{ | ||
| Description: "Input streams mapped by stream ID.", | ||
| Optional: true, |
There was a problem hiding this comment.
The streams attribute is now optional-only (no longer computed), but there's no default value specified. Consider whether a default should be added for consistency with the previous schema behavior, or document why the computed+default was intentionally removed.
| Optional: true, | |
| Optional: true, | |
| Default: objectdefault.StaticValue(basetypes.NewMapNull(types.ObjectType{AttrTypes: getInputStreamNestedObject(varsAreSensitive).Attributes})), |
| body.Inputs = utils.MapRef(model.toAPIInputsFromInputsAttribute(ctx, &diags)) | ||
| } | ||
|
|
||
| body.Inputs = model.toAPIInputsFromInputsAttribute(ctx, &diags) |
There was a problem hiding this comment.
The function now always returns a pointer to a map, but the previous logic conditionally set body.Inputs only when inputs were known and non-empty. This change means empty inputs {} will always be sent to the API. Verify this is the intended behavior and doesn't cause issues with the Fleet API.
| body.Inputs = model.toAPIInputsFromInputsAttribute(ctx, &diags) | |
| inputs := model.toAPIInputsFromInputsAttribute(ctx, &diags) | |
| if inputs != nil && len(*inputs) > 0 { | |
| body.Inputs = inputs | |
| } |
| for _, stream := range input.Streams.Elements() { | ||
| streamModel := integrationPolicyInputStreamModel{} | ||
| d := stream.(types.Object).As(ctx, &streamModel, basetypes.ObjectAsOptions{}) |
There was a problem hiding this comment.
The type assertion stream.(types.Object) could panic if the element is not a types.Object. Consider adding a type check before the assertion or using the comma-ok idiom to safely handle unexpected types.
| for _, stream := range input.Streams.Elements() { | |
| streamModel := integrationPolicyInputStreamModel{} | |
| d := stream.(types.Object).As(ctx, &streamModel, basetypes.ObjectAsOptions{}) | |
| for i, stream := range input.Streams.Elements() { | |
| streamModel := integrationPolicyInputStreamModel{} | |
| obj, ok := stream.(types.Object) | |
| if !ok { | |
| diags.AddError( | |
| fmt.Sprintf("Unexpected stream type at index %d", i), | |
| fmt.Sprintf("Expected types.Object, got %T", stream), | |
| ) | |
| return false, diags | |
| } | |
| d := obj.As(ctx, &streamModel, basetypes.ObjectAsOptions{}) |
Related to #617
Fixes #1415
Fixes #889
This PR aims to allow users to only provide streams/vars for integration policies where they differ from the defaults for a specific integration. In reality, users have been attempting to do just that, but hitting some form of inconsistent state error. These errors should no longer occur as we're taking into account the integration defaults when comparing the user config and the API derived state.