- Notifications
You must be signed in to change notification settings - Fork 123
Only require any input parameters to be specified if they differ from the defaults#1558
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading. Please reload this page.
Conversation
tobio commented Dec 16, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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, |
CopilotAIDec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
CopilotAIDec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) | |
| ifinputs!=nil&&len(*inputs) >0{ | |
| body.Inputs=inputs | |
| } |
Uh oh!
There was an error while loading. Please reload this page.
| for_, stream:=rangeinput.Streams.Elements(){ | ||
| streamModel:=integrationPolicyInputStreamModel{} | ||
| d:=stream.(types.Object).As(ctx, &streamModel, basetypes.ObjectAsOptions{}) |
CopilotAIDec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:=rangeinput.Streams.Elements(){ | |
| streamModel:=integrationPolicyInputStreamModel{} | |
| d:=stream.(types.Object).As(ctx, &streamModel, basetypes.ObjectAsOptions{}) | |
| fori, stream:=rangeinput.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), | |
| ) | |
| returnfalse, 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.