-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
command/format: minor adjustments to plan rendering #15362
Conversation
Lexicographic sorting by the string form produces the wrong result because [9] sorts after [10], so this custom comparison function takes that into account and compares each portion separately to get a more intuitive result.
This is a specialized thin wrapper around parseResourceAddressInternal that can be used to obtain a ResourceAddress from the keys in ModuleDiff.Resources. This is not something we'd ideally expose, but since the internal address format is already exposed in the ModuleDiff object this ends up being necessary to process the ModuleDiff from other packages, e.g. for display in the UI.
This change makes various minor adjustments to the rendering of plans in the output of "terraform plan": - Resources are identified using the standard resource address syntax, rather than exposing the legacy internal representation used in the module diff resource keys. This fixes #8713. - Subjectively, having square brackets in the addresses made it look more visually "off" when the same name but with different indices were shown together with differing-length "symbols", so the symbols are now all padded and right-aligned to three characters for consistent layout across all operations. - The -/+ action is now more visually distinct, using several different colors to help communicate what it will do and including a more obvious "(new resource required)" marker to help draw attention to this not being just an update diff. This fixes #15350. - The resources are now sorted in a manner that sorts index [10] after index [9], rather than after index [1] as we did before. This makes it easier to scan the list and avoids the common confusion where it seems that there are only 10 items when in fact there are 11-20 items with all the tens hiding further up in the list.
case len(addr.Path) < len(other.Path): | ||
return true | ||
|
||
case !reflect.DeepEqual(addr.Path, other.Path): |
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.
I don't think it is right now, but is it possible for indexes to end up in a path? Might they be if we can add count
support to modules?
This is probably fine for now, I'm only mentioning it because I think I've fixed "lexically sorted numbers" 2 or 3 times in the past couple releases.
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.
Indeed in future this may get more complex, but I expect that at that point we rewrite all of this ResourceAddress
code, since we will have hit the limit of what can be represented by the current model.
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
This change makes various minor adjustments to the rendering of plans in the output of
terraform plan
:Resources are identified using the standard resource address syntax, rather than exposing the legacy internal representation used in the module diff resource keys. This fixes terraform import command fails when there are several nested levels on the resource address #8713.
Subjectively, having square brackets in the addresses made it look more visually "off" when the same name but with different indices were shown together with differing-length "symbols", so the symbols are now all padded and right-aligned to three characters for consistent layout across all operations.
The
-/+
action is now more visually distinct, using several different colors to help communicate what it will do and including a more obvious(new resource required)
marker to help draw attention to this not being just an update diff. This fixes Stop colouring a destruction of a resource in green #15350.The resources are now sorted in a manner that sorts index
[10]
after index[9]
, rather than after index[1]
as we did before. This makes it easier to scan the list and avoids the common confusion where it seems that there are only 10 items when in fact there are 11-20 items with all the tens hiding further up in the list.Certainly there are many more significant changes that could be made in this area, but this is just intended to be a small, low-risk change to deal with some minor annoyances.
Here's a screenshot, though it's made to look more weird by the strange mismatching saturation levels I apparently have configured in my terminal:
These are all just regular primary VT100 colors in practice.