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

Sort IAM policy bindings by their role name to get simpler diffs #3855

Merged
merged 2 commits into from
Jun 24, 2019

Conversation

pdecat
Copy link
Contributor

@pdecat pdecat commented Jun 17, 2019

This should resolve #3728

@pdecat
Copy link
Contributor Author

pdecat commented Jun 17, 2019

Can go a step further and sort members of each binding too.

@pdecat pdecat force-pushed the sort-iam-policy-bindings branch from 3159d0d to 7840103 Compare June 17, 2019 15:08
@pdecat
Copy link
Contributor Author

pdecat commented Jun 17, 2019

An alternative option may be to convert all bindings and members attributes from lists to sets but that's way more intrusive.

@danawillow danawillow self-requested a review June 18, 2019 22:54
@pdecat pdecat force-pushed the sort-iam-policy-bindings branch 2 times, most recently from 7e67914 to fc25f8a Compare June 19, 2019 23:11
@danawillow
Copy link
Contributor

Will this show diffs for users that already have iam policies defined in their terraform configs?

@pdecat
Copy link
Contributor Author

pdecat commented Jun 22, 2019

No it does not.

@pdecat pdecat force-pushed the sort-iam-policy-bindings branch from fc25f8a to e794a41 Compare June 22, 2019 08:16
@pdecat
Copy link
Contributor Author

pdecat commented Jun 22, 2019

The resources where this datasource is used are already suppressing the diffs with the jsonPolicyDiffSuppress() function: https://github.com/terraform-providers/terraform-provider-google/blob/e9aebb8b158646bd1d9b4fbe9854734cd8c09b52/google/resource_google_project_iam_policy.go#L190

@pdecat
Copy link
Contributor Author

pdecat commented Jun 22, 2019

Let me provide some examples to demonstrate the result of this change.

@pdecat
Copy link
Contributor Author

pdecat commented Jun 22, 2019

Here's a test case configuration: https://gist.github.com/pdecat/4450fbbee34074d9df9cdb56ceac6772

After having applied that configuration with v2.9.1 and commenting line 86, here's the plan: https://gist.github.com/pdecat/fd84253c753fa9c20b8995e699da07b4

With this modification, here's the plan: https://gist.github.com/pdecat/629a8d7e733122737002e02bf2f92964

Copy link
Contributor

@danawillow danawillow left a comment

Choose a reason for hiding this comment

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

Thanks for the detailed clarification!

@ghost
Copy link

ghost commented Jul 25, 2019

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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

@ghost ghost locked and limited conversation to collaborators Jul 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve google_iam_policy diff formatting
2 participants