search

coinbase / terraform-landscape

Improve Terraform's plan output to be easier to read and understand
Apache License 2.0
1.59k stars 116 forks source link

sort lists of key value pairs #75

Closed david-chenevert closed 5 years ago

david-chenevert commented 6 years ago

This PR improves diff output quality, when a list of key-value-pair changes.

This issue is described in more detail here:

https://github.com/coinbase/terraform-landscape/issues/74

sds commented 6 years ago

Hey @david-chenevert, thanks for the PR.

I'm not sure about this change. While it may certainly be the case for many AWS-related resources that the order of the list of KV pairs does not matter, we cannot say this is always the case.

We would strongly prefer a plugin-based approach as discussed in #70 where you can specify an output formatter for each resource type as you desire, rather than make this apply globally to all possible resource types.

tomgoren commented 6 years ago

@sds I thought this just enhances the original sorting implementation.

Can you explain the conflict, or alternatively, show an example of how this would be provided as a plugin? I don't see any docs or references available that describe existing plugins.

Thanks!

sds commented 6 years ago

This pull request modifies the behavior of the previous sorting implementation.

The previous implementation did not sort lists (notice it uses map, not sort), only keys within hashes.

What this PR does is explicitly start sorting lists. While this may be fine for many use cases, order can matter when dealing with lists in the general case.

david-chenevert commented 6 years ago

Yes, you are right. This does modify behavior in a way that could be detrimental in some case.

Although I think that probability is pretty low, it is not exactly zero.

Can you explain or provide a link about creating a custom plugin? I looked at the PR you mentioned ( https://github.com/coinbase/terraform-landscape/issues/70), but I did not understand if plugins are currently implemented, vs, plugins would be a nice feature.

Another approach, other than a plugin, would be to allow this change to be committed, but controlled by a flag. The default would be existing (safe) behavior. Would you be open to that?

Thank you so much for responding to my PR,

David

On Wed, Nov 14, 2018 at 8:03 AM Shane da Silva notifications@github.com wrote:

This pull request modifies the behavior of the previous sorting implementation.

The previous implementation did not sort lists https://github.com/coinbase/terraform-landscape/commit/afb2a4ed0263b6c34edf1e613fa215328c1a2bfd#diff-d7a30e04fa50ed5e74c051fba2fece2bR127 (notice it uses map, not sort), only keys within hashes https://github.com/coinbase/terraform-landscape/commit/afb2a4ed0263b6c34edf1e613fa215328c1a2bfd#diff-d7a30e04fa50ed5e74c051fba2fece2bR129 .

What this PR does is explicitly start sorting lists. While this may be fine for many use cases, order can matter when dealing with lists in the general case.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/coinbase/terraform-landscape/pull/75#issuecomment-438716109, or mute the thread https://github.com/notifications/unsubscribe-auth/AeDqCqhqIK4WRtnOF2syD70VBW7rkU2-ks5uvD7NgaJpZM4YLziP .

sds commented 6 years ago

Hey @david-chenevert, someone on the Terraform development team mentioned it will include support for better formatting soon. I would suggest we wait to see what happens before going too far with building custom formatters. Thanks!

david-chenevert commented 6 years ago

That is good to hear. Yes I agree let's wait to see what they change. For the time being I'm running my branch locally, which is fine.

On Fri, Nov 30, 2018 at 12:50 PM Shane da Silva notifications@github.com wrote:

Hey @david-chenevert https://github.com/david-chenevert, someone on the Terraform development team mentioned it will include support for better formatting soon. I would suggest we wait to see what happens before going to far with building custom formatters. Thanks!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/coinbase/terraform-landscape/pull/75#issuecomment-443335222, or mute the thread https://github.com/notifications/unsubscribe-auth/AeDqClgf6pe0Jf-0UUNVr83DQxGp853oks5u0Zn_gaJpZM4YLziP .

dharamsk commented 5 years ago

what about having a flag to sort KV pairs? This would be incredibly useful to us, given we don't use anything in terraform that requires strict ordering of these resources.