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

cli: Fix diff for nested set unchanged elements #29983

Merged
merged 2 commits into from
Nov 19, 2021

Conversation

alisdair
Copy link
Contributor

@alisdair alisdair commented Nov 19, 2021

Unchanged elements in nested attributes backed by sets were previously misrendered as empty objects. This PR removes the additional brackets and adds a count of unchanged elements, and secondarily fixes the indentation for these diffs.

Targeting 1.1 backport.

Screenshots

Before:

after

After:

Screen Shot 2021-11-19 at 1 04 25 PM

Unchanged elements in nested attributes backed by sets were previously
misrendered as empty objects. This commit removes the additional
brackets and adds a count of unchanged elements.
@alisdair alisdair added cli 1.1-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged labels Nov 19, 2021
@alisdair alisdair requested a review from a team November 19, 2021 17:01
@alisdair alisdair self-assigned this Nov 19, 2021
Copy link
Contributor

@apparentlymart apparentlymart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Thanks for digging into this.


Looking at your "after" screenshot it still felt a little "off", though it did take me staring at it for quite a few minutes before I realized what was throwing me:

It seems like all of the lines after the opening - { are missing one level of indentation, so the nested -s are directly below the brace rather than indented relative to it.

While I guess it's a matter of opinion whether it's a "good thing", in all other cases we have created the effect of each additional nesting level adding four columns of indent and then having the change symbols ~ - + live inside the left "gutter" when they are present, as if they are annotations in the margin rather than part of the actual content. Notice for example how the closing ] for ~ values = [ lines up with values rather than with ~.

It'd be totally reasonable to treat that as out of scope for what you were aiming to solve here, so I'm not intending this as a blocker for merging, but if it's something you can see an easy path to fix while you're there it'd be nice to make this consistent with the formatting elsewhere.

@@ -2859,6 +2867,7 @@ func TestResourceChange_nestedSet(t *testing.T) {
- {
- mount_point = "/var/diska" -> null
},
# (1 unchanged element hidden)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GitHub won't let me properly bracket the unchanged lines here to make this a real "suggestion" but here's a proposed alternative expected output for the indentation I was talking about in my review comment:

  # test_instance.example will be updated in-place
  ~ resource "test_instance" "example" {
      ~ ami   = "ami-BEFORE" -> "ami-AFTER"
      ~ disks = [
          + {
              + mount_point = "/var/diska"
              + size        = "50GB"
            },
          - {
              - mount_point = "/var/diska" -> null
            },
          # (1 unchanged element hidden)
        ]
        id    = "i-02ae66f368e8518a9"

      + root_block_device {
          + new_field   = "new_value"
          + volume_type = "gp2"
        }
      - root_block_device {
          - volume_type = "gp2" -> null
        }
    }

(That was kinda awkward to edit in the GitHub comment box so I probably didn't get it all exactly right, but the indentation of the disks contents is the main thing I'm getting at here.)

@alisdair
Copy link
Contributor Author

Great catch! The indentation fix seems pretty simple to me, so I've rolled it into this PR. Description & screenshot updated too.

I also quickly checked that the other nested attribute types are indented correctly, and to me they look right:

image

@alisdair alisdair merged commit ec3058d into main Nov 19, 2021
@alisdair alisdair deleted the alisdair/fix-nested-set-unchanged branch November 19, 2021 18:40
@apparentlymart
Copy link
Contributor

apparentlymart commented Nov 19, 2021

Thanks!

The second example in your screenshot still seems a little off: the + number = 5 line has the + directly under the " of "baz" rather than indented (under the a), and the closing }, is directly under the ~ rather than under the " of "baz".

With that said, still potentially out of scope for what you're doing here so would be totally reasonable to just open a new issue to represent that and fix it up another time, but I'm hoping 🤞 that the pretty simple fix you found for the tuple/list case is also pretty simple for the object/map case. 😁

@alisdair
Copy link
Contributor Author

alisdair commented Nov 19, 2021

🤦 Thanks for catching that too. I had already merged this PR so I've opened #29986 to hopefully address it. The same issue looked to be the case for the nested single value, so I adjusted that too.

@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
1.1-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged cli
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants