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

gitlab_user: Do not set skip_confirmation on read #491

Merged
merged 3 commits into from
Jan 24, 2022

Conversation

armsnyder
Copy link
Collaborator

@armsnyder armsnyder commented Nov 23, 2020

Follow-up on #490 @roidelapluie

I looked at it again, and I don't think skip_confirmation should ever be set during reads or imports, since it is an attribute that only has effect during initial creation. (API ref)

This change should fix an issue where, if the resource is created with skip_confirmation=false, then when the user confirms their email, it would result in an unnecessary resource diff of skip_confirmation=true.

@armsnyder armsnyder changed the title gitlab_user: Do not calculate skip_confirmation gitlab_user: Do not set skip_confirmation on read Nov 23, 2020
@roidelapluie
Copy link
Collaborator

If we do not set it on read, it means that it will always be flapping if a user sets it to true.

We might need a suppressdiff function?

@armsnyder
Copy link
Collaborator Author

Hmm, I thought if we don't d.Set() it, then it will stay whatever it was originally set to.

@roidelapluie
Copy link
Collaborator

Hmm, I thought if we don't d.Set() it, then it will stay whatever it was originally set to.

It will not change the user input on import, would it?

@roidelapluie
Copy link
Collaborator

roidelapluie commented Nov 23, 2020

The correct behaviour would be:

set it to computed + force_new

@armsnyder
Copy link
Collaborator Author

Updated. That seemed to work.

@armsnyder armsnyder marked this pull request as draft November 23, 2020 09:52
@armsnyder armsnyder marked this pull request as ready for review November 23, 2020 10:17
@armsnyder
Copy link
Collaborator Author

Let's not merge this until #493 is resolved, since I did not run these tests myself locally.

@armsnyder armsnyder marked this pull request as draft November 30, 2020 07:07
Copy link
Member

@timofurrer timofurrer left a comment

Choose a reason for hiding this comment

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

Needs a rebase, but I think otherwise LGTM. The referenced blocker has also been resolved some time ago.
What do you think @armsnyder ?

gitlab_user tests refactor: Remove Password and SkipConfirmation fields from testAccGitlabUserExpectedAttributes struct

These fields were not being checked in testAccCheckGitlabUserAttributes because they cannot be read.

gitlab_user: Make skip_confirmation ForceNew, and more tests
@armsnyder armsnyder marked this pull request as ready for review January 24, 2022 07:27
@armsnyder armsnyder requested a review from timofurrer January 24, 2022 07:27
@armsnyder armsnyder merged commit edda219 into master Jan 24, 2022
@armsnyder armsnyder deleted the bugfix/user-npe-2 branch January 24, 2022 07:56
@armsnyder armsnyder added this to the 3.9.0 milestone Jan 26, 2022
@github-actions
Copy link

github-actions bot commented Feb 4, 2022

This functionality has been released in 3.9.0 of the Terraform GitLab Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue. Thank you!

@github-actions github-actions bot locked and limited conversation to collaborators Nov 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

3 participants