Skip to content
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

fix(diff): switch from SetNestedAttribute to MapNestedAttribute to resolve diff issues #7

Merged
merged 7 commits into from
Jan 25, 2023

Conversation

Integralist
Copy link
Owner

@Integralist Integralist commented Jan 24, 2023

Problem

The original design for 'domains' used a nested attribute of a Set type (similar to the original Terraform provider) and this causes Terraform to generate inaccurate diffs.

An example of this can be seen in the below screenshot. I had two domains already in my state, the first domain was updated to have a comment, and the second domain had its name changed slightly. The generated diff correctly shows the second domain as needing to be deleted and a new domain with -updated in its name needing to be created BUT the first domain is incorrectly showing as needing to be deleted and recreated (which isn't what is actually happening as internally we make a single 'update' API call to add the comment to the existing domain resource):

Screenshot 2023-01-24 at 17 30 16

Solution

I'm switching to a nested map attribute type, and although more verbose (for the user) in Terraform config syntax, the change actually helps simplify a bunch of domain logic (such as not having to manually identify a 'key' to be calculated into a hash, which in the case of the domain resource was its name field, and now that 'key' is automatically handled simply by the nature of the design of a map type).

Here is my initial plan:

Screenshot 2023-01-25 at 10 16 33

Here is the plan after making modifications:

Screenshot 2023-01-25 at 10 19 02

Notice how the generated diff is more accurate. It can recognise that a single field (comment in this case) is being modified 'in-place' and so we're not showing the first domain as needing to be deleted and recreated but simply needing to be modified.

Extra

This change also makes abstracting the algorithm used for calculating changes easier. Meaning, we should be able to make a function that can abstract this logic so it can be reused across different resource types (e.g. backends).

Interface Changes

We're moving from config that looks like this (SetNestedAttribute):

domains = [
    {
        name = "example-1.integralist.co.uk"
        comment = "a comment"
    },
    {
        name = "example-2.integralist.co.uk"
    },
]

To config that looks like this (MapNestedAttribute):

domains = {
    "example-1.integralist.co.uk" = {
        comment = "a comment"
    },
    "example-2.integralist.co.uk" = {},
}

@Integralist Integralist added the bug Something isn't working label Jan 24, 2023
@Integralist Integralist force-pushed the integralist/fix-diff-issues branch from 242e4fc to e11bb4f Compare January 24, 2023 19:55
@Integralist Integralist changed the title fix(diff): switch from set type to map to avoid diff issues fix(diff): switch from SetNestedAttribute to MapNestedAttribute to resolve diff issues Jan 25, 2023
@Integralist Integralist merged commit b012ccd into main Jan 25, 2023
@Integralist Integralist deleted the integralist/fix-diff-issues branch January 25, 2023 11:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant