Suppress diff on the skip_confirmation attribute of gitlab_user

[PatrickRice-KSC]

Description

Fix an issue with gitlab_user where the skip_confirmation attribute results in a non-empty plan on every run.

Closes #1101

PR Checklist Acknowledgement

• [x] I acknowledge that all of the following items are true, where applicable:
  • Resource attributes match 1:1 the names and structure of the API resource in the GitLab API documentation.
  • Examples are updated with:
    • A *.tf file for the resource/s with at least one usage example
    • A *.sh file for the resource/s with an import example (if applicable)
  • New resources have at minimum a basic test with three steps:
    • Create the resource
    • Update the attributes
    • Import the resource
  • No new //lintignore comments were copied from existing code. (Linter rules are meant to be enforced on new code.)

[timofurrer]

Thanks for taking the initiative to implement this @PatrickRice-KSC !

However, I'm not yet convinced that this is the best solution. Imagine the following scenario:

1. Creating a user with skip_confirmation = true
2. Realising that this was wrong and the user must confirm the email
3. Changing skip_confirmation = false
4. ?

Now, since this works from a tf perspective the user may assume that the change went through and the user will have to confirm the email, but that's actually wrong. The provider just silently ignored the change because of the diff ..

What I'm thinking is that, is this really "always" an issue or just an issue after an import? Maybe for the normal use case, when a user just flips the skip_confirmation we should actually set the skip_reconfirmation flag instead? (I'm not yet 100% sure when a reconfirmation is triggered) ... although this may not work for the scenario I've described above either..

Maybe it just makes sense to investigate what it would mean and if it's even possible to expose this attribute in the GitLab read APIs?

[PatrickRice-KSC]

@timofurrer - I'm not really proficient in ruby, so I can't speak to how possible it is to expose the API via gitlab 🙂 .

This is a problem with more than just import though, it'll happen any time a value is set into that field in a config, because we never set the field into state anywhere (likely because it's not returned from any of the APIs). You can see that code here: https://github.com/gitlabhq/terraform-provider-gitlab/blob/main/internal/provider/resource_gitlab_user.go#L123

We could potentially set it into state in create/update and assert that the value got set appropriately during insert/update, but that seems a bit of an anti-pattern since we've seen instances before where the GitLab API returned 200 and didn't actually set the value, so we wouldn't want to give the impression that an operation succeeded when it didn't. I debated seeing if we could use getChange, but that compares state to value, which means since the state is always nil it still comes back with a change. I also debated whether there was something more elegant than just return true for the skip func, but it's all comparing to existing state which would be nil :-/

[timofurrer]

This is a problem with more than just import though, it'll happen any time a value is set into that field in a config, because we never set the field into state anywhere (likely because it's not returned from any of the APIs).

We could potentially set it into state in create/update and assert that the value got set appropriately during insert/update,

Yes, I guess we meant the same here (I was just assume that and wasn't specific enough, sorry) ... basically:

It would require that we set the skip_confirmation attribute from the config to state during create / update - similar to what we do with a token. In the update we would just set the value from the config every time (because we don't know if there was a drift).

Then, there is still the import ... for which we may just use what's in the config ...

I'll dig into GitLab once I have some time.

[github-actions[bot]]

Marking this pull request as stale due to 30 days of inactivity. If this pull request receives no comments in the next 14 days it will be closed. Maintainers can also remove the stale label.

To help this pull request get reviewed, please check that it is rebased onto the latest and is passing automated checks. It also helps if you could reference an issue that the pull request resolves, and create one if it doesn't exist.

[olhado]

Just poking this so it stays alive. The lifecycle ignore_changes workaround is not great.

[PatrickRice-KSC]

@timofurrer - Should we just update this to set the state on create/update, and skip import verification for now? It would require a separate attribute to change in order to trigger an "update" operation because we'd still need the skip func, but at least it's slightly more graceful.

[timofurrer]

@PatrickRice-KSC I still didn't have any time to dig into the GitLab side of things ...

Should we just update this to set the state on create/update, and skip import verification for now?

Yes, I'd skip import for now ..

[PatrickRice-KSC]

@timofurrer - I updated the way this is handled to be set directly from the config on create/update, removed the suppress function and set the attribute as computed.

I also updated the test to check the value directly since the attribute doesn't exist on the *gitlab.User object (since it can't be retrieved from any API).

[github-actions[bot]]

Marking this pull request as stale due to 30 days of inactivity. If this pull request receives no comments in the next 14 days it will be closed. Maintainers can also remove the stale label.

To help this pull request get reviewed, please check that it is rebased onto the latest and is passing automated checks. It also helps if you could reference an issue that the pull request resolves, and create one if it doesn't exist.

[github-actions[bot]]

Marking this pull request as stale due to 30 days of inactivity. If this pull request receives no comments in the next 14 days it will be closed. Maintainers can also remove the stale label.

To help this pull request get reviewed, please check that it is rebased onto the latest and is passing automated checks. It also helps if you could reference an issue that the pull request resolves, and create one if it doesn't exist.

[Shocktrooper]

@PatrickRice-KSC Is this good for a review?

[PatrickRice-KSC]

@Shocktrooper - it is, but I know @timofurrer had some reservations with the approach

[github-actions[bot]]

Marking this pull request as stale due to 30 days of inactivity. If this pull request receives no comments in the next 14 days it will be closed. Maintainers can also remove the stale label.

To help this pull request get reviewed, please check that it is rebased onto the latest main and is passing automated checks. It also helps if you could reference an issue that the pull request resolves, and create one if it doesn't exist.

[PatrickRice-KSC]

IHMO, we could actually merge it given it provides some value for some people already 😊

However, what would you think about just suppressing any change on this field?

Like we do here with the skip_wait_for_default_branch_protection attribute:

Hey @timofurrer - I'm working through fixing the test failures, and when we set the DiffSuppressFunc to just return true, the change no longer works how we want it to, because it suppresses the initial creation diff as well, and the value always returns false. So I think we need to leave the DiffSuppressFunc off, and just let it set the value into state on the first apply. Thoughts?

[github-actions[bot]]

Thank you very much for your contribution :heart:

The GitLab Terraform Provider project has been moved to GitLab and we welcome your contribution there :tada: