Skip to content

Set attribute defaults conditionally #259

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

wfdewith
Copy link

@wfdewith wfdewith commented Jun 4, 2025

Rationale

(Based on discoveries in #257.)

When importing resources from backup files (or an ISO file in case of storage volumes) or copying them from existing resources, Incus and therefore the provider will determine ('compute' in Terraform terms) a number of attributes from the source file or resource, instead of the Terraform configuration. In the current state of the provider, this leads to two problems:

  • Users can set these attributes (e.g. config) together with the source_file (or source_volume, etc.) in their configuration. When the provider then 'computes' these attributes based on the source, their values may not match the user's configuration. This leads to errors such as the following.
    ╷
    │ Error: Provider produced inconsistent result after apply
    │
    │ When applying changes to incus_storage_volume.some_backup, provider
    │ "provider[\"registry.terraform.io/lxc/incus\"]" produced an unexpected new value: .config: new element
    │ "security.shared" has appeared.
    │
    │ This is a bug in the provider, which should be reported in the provider's own issue tracker.
    ╵
    
  • The attributes have default values in the provider. Unfortunately, Terraform applies these defaults unconditionally, meaning there is no determinable difference between a user omitting an attribute or setting the default value in their configuration. This loops back to the first problem: when the default value of an attribute does not match what is in the source file or resource, Terraform produces similar errors to the one mentioned above.

Solving the first problem is straightforward: we can just make the offending attributes mutually exclusive with source_file (or source_volume, etc.) with validators.ConflictsWith.

The second problem is unfortunately harder to solve. We have to remove the defaults from the attributes and only set them if source_file (or source_volume, etc.) are not specified. There are broadly two ways to achieve this, as far as I can see:

  • Just set the defaults somewhere in the provider code, moments before they are used. In my opinion, this is not a good solution as it will scatter the default values throughout the provider code base for the specific resource.
  • Use plan modifiers, which are executed after validation but before application of the configuration. We can use a plan modifier to determine which attributes are configured and only set the default value of an attribute if source_file (or source_volume, etc.) are not configured. This is the solution I went with in this PR. One small disadvantage is that plan modifiers need to be implemented per attribute type, so there is some duplication.

TODO

  • Fix the problem in incus_storage_volume.
  • Write tests for incus_storage_volume.
    • source_file
    • source_volume
  • Determine which resources have the same problem and write tests and fixes for them as well.
    • incus_storage_bucket
    • incus_image
    • incus_instance

@wfdewith wfdewith force-pushed the conditional-default branch 2 times, most recently from 83e1780 to cdda6cd Compare June 6, 2025 16:08
@wfdewith
Copy link
Author

wfdewith commented Jun 6, 2025

This should cover all occurrences of this bug in the provider. I made a few extra changes:

  • All relevant tests Inow use the null_resource trick for creating backup files that was already used in the resource_instance_test.go file. This should ensure better resource cleanup when tests fail compared to just running incus commands.
  • incus_instance already had some of the conflicts between attributes and source_file/source_instance configured, however, these validators were configured on the source_file and source_instance attributes. I moved them to the attributes they reference because that puts them next to the new plan modifiers, otherwise this code has to be kept in sync in two different places.

@maveonair Let me know what you think!

@wfdewith wfdewith force-pushed the conditional-default branch from cdda6cd to f3e75d0 Compare June 6, 2025 16:47
@maveonair
Copy link
Member

* All relevant tests Inow use the `null_resource` trick for creating backup files that was already used in the `resource_instance_test.go` file. This should ensure better resource cleanup when tests fail compared to just running `incus` commands.

Nice!

* `incus_instance` already had some of the conflicts between attributes and `source_file`/`source_instance` configured, however, these validators were configured on the `source_file` and `source_instance` attributes. I moved them to the attributes they reference because that puts them next to the new plan modifiers, otherwise this code has to be kept in sync in two different places.

LGTM and I am looking forward to review the pull request when it is ready.

@wfdewith
Copy link
Author

Tests are failing because of this bug: lxc/incus#2185

@wfdewith wfdewith force-pushed the conditional-default branch from f3e75d0 to d3d2e82 Compare June 10, 2025 09:55
@wfdewith wfdewith force-pushed the conditional-default branch from d3d2e82 to 09ecc61 Compare June 10, 2025 09:57
@wfdewith
Copy link
Author

The CI failure appears unrelated to my changes; test failures are all in code I didn't touch in this PR.

@wfdewith wfdewith marked this pull request as ready for review June 10, 2025 10:44
@wfdewith wfdewith force-pushed the conditional-default branch from 09ecc61 to be031cd Compare June 11, 2025 16:05
@wfdewith
Copy link
Author

I've reordered the commits so that the tests come after the changes to the code to prevent commits with failing tests on main. It's now ready for review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants