Skip to content

Users: ignore case when diffing email and login #1512

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

Merged
merged 4 commits into from
Apr 25, 2024

Conversation

IevaVasiljeva
Copy link
Contributor

What

This PR makes user login and email diffing case insensitive.

Why

We've changed the implementation on Grafana's side to be case insensitive when resolving user emails and logins.

This already causes a diff for newly created users if upper case characters are used for user logins or emails, and will cause an even larger diff when grafana/grafana#86359 is merged, as we will be migrating all existing user logins and emails to lower case.

How to test

  • use Grafana TF provider without this change;
  • create a user with upper case characters in login/email;
  • run terraform apply again and observe the diff;
  • now use Grafana TF provider with this change and run terraform apply - no diff should appear.

User resource used:

resource "grafana_user" "test_user" {
  email    = "[email protected]"
  login    = "nEW_terRAForm_user"
  password = "password"
}

Diff observed:

  # grafana_user.test_user will be updated in-place
  ~ resource "grafana_user" "test_user" {
      ~ email    = "[email protected]" -> "[email protected]"
        id       = "15"
      ~ login    = "[email protected]" -> "[email protected]"
        # (3 unchanged attributes hidden)
    }

@IevaVasiljeva IevaVasiljeva requested a review from a team as a code owner April 25, 2024 10:58
Copy link

In order to lower resource usage and have a faster runtime, PRs will not run Cloud tests automatically.
To do so, a Grafana Labs employee must trigger the cloud acceptance tests workflow manually.

@@ -96,6 +108,16 @@ resource "grafana_user" "test" {
}
`

const testAccUserConfig_mixedCase = `
Copy link
Contributor

Choose a reason for hiding this comment

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

could potentially add a case for all uppercase

only for all coverage

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, sorry, didn't see this comment before merging. I think in this case it doesn't matter enough to raise another PR just for that.

@IevaVasiljeva IevaVasiljeva merged commit f25ee0c into main Apr 25, 2024
27 checks passed
@IevaVasiljeva IevaVasiljeva deleted the ieva/user-case-insensitivity branch April 25, 2024 13:28
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.

3 participants