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

Sorting permissions object on username/group name before storing. #1311

Merged
merged 5 commits into from
Mar 16, 2021

Conversation

sumitAgrawal007
Copy link
Contributor

@sumitAgrawal007 sumitAgrawal007 commented Jan 13, 2021

Description

The permissions objects received from server are not necessarily sorted on username/groupname. We are asking the users to keep it sorted in the TF file and this PR sorts the received objects alphabetically so that terraform doesn't show a diff while refreshing.

Acceptance tests

  • Have you added an acceptance test for the functionality being added?
  • Have you run the acceptance tests on this branch?

Release Note

Release note for CHANGELOG:


- resource/vsphere_entity_permissions: Sorting permission objects on username/groupname before storing. (#1311)

References

Closes #1293

@ghost ghost added size/xs Relative Sizing: Extra-Small documentation Type: Documentation labels Jan 13, 2021
@koikonom
Copy link
Contributor

Hey @sumitAgrawal007, I am trying to understand this change and I think I need some additional context.

If I understand correctly the only thing this PR does it to sort the list of roles before setting it in the Read() function. Is there any actual effect on vSphere by this change?

What I'm concerned is that if users ignored the advice and did not sort their roles alphabetically, then once they run this version of the provider their plan will reports that changes are required, which may or may not be true.

We could use a diff suppress func where we sort the list in the config and the list we get from vsphere and if they're equal then we just don't report back as a difference but, again, I need your help to understand the consequences of such a thing.

@koikonom koikonom added the waiting-response Status: Waiting on a Response label Feb 12, 2021
@sumitAgrawal007
Copy link
Contributor Author

No, there is no affect on vsphere by this change.

The reason of this change is that even if the user sorts his persmission in the configuration file, still the vsphere server returns them in unsorted/arbit way, and terraform plan reports that there are going to be changes.
Earlier I had assumed that the server returns them in sorted way, which is true in many cases, but there was a bug raised, and he showed me thats not always the case.

So the best way to handle these issues is to suggest the user to sort them in .tf file and sort the server response as well.
We can use a diff surpress function to surpress diff if there is no change, but that wont show the diff correctly if there is a change. The unsorted permissions lists diff will be very hapazard.

So I am also going to add a diff surpress function here, to make it complete.

@ghost ghost removed waiting-response Status: Waiting on a Response labels Feb 15, 2021
@ghost ghost added size/s Relative Sizing: Small and removed size/xs Relative Sizing: Extra-Small labels Feb 15, 2021
Copy link
Contributor

@appilon appilon left a comment

Choose a reason for hiding this comment

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

Had a quick question about the slice trick I noticed? Other than that LGTM!

@appilon appilon merged commit a887f82 into hashicorp:master Mar 16, 2021
@ghost
Copy link

ghost commented Apr 16, 2021

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 as resolved and limited conversation to collaborators Apr 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Type: Documentation size/s Relative Sizing: Small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

vsphere_entity_permissions changed after every plan, even when specified in alphabetical order
3 participants