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

Change user lists to sets #116

Merged
merged 3 commits into from
Oct 20, 2020
Merged

Conversation

medains
Copy link
Contributor

@medains medains commented Sep 17, 2020

Users as lists may not be reported back from grafana in the same order - resulting in continually reported resource changes.

Treating the users as sets instead deals with this nicely.

@ghost ghost added the size/XS label Sep 17, 2020
Copy link
Member

@trotttrotttrott trotttrotttrott left a comment

Choose a reason for hiding this comment

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

Thanks, @medains. I agree with this change, but it breaks some acceptances tests because we can't read elements from sets as the tests are trying to do. Could you please update them? Removing the following should do it:

Also please remove the following from the organization resource docs:

Note - Users specified for each role-group (admins, editors, viewers) should be listed in ascending alphabetical order (A-Z). By defining users in alphabetical order, Terraform is prevented from detecting unnecessary changes when comparing the list of defined users in the resource to the (ordered) list returned by the Grafana API.

@medains
Copy link
Contributor Author

medains commented Oct 16, 2020

Updated as requested

Copy link
Member

@trotttrotttrott trotttrotttrott left a comment

Choose a reason for hiding this comment

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

Awesome. Thanks, @medains 👍

@trotttrotttrott trotttrotttrott merged commit 7d16492 into grafana:master Oct 20, 2020
@medains medains deleted the fix-users branch November 18, 2020 08:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants