Handling environmentScope as a key for project variables

[techthumb]

Gitlab introduced the environmentScope attribute to ProjectVariables.

Within Gitlab, ProjectVariables are currently keyed by key.

Since the introduction of environmentScope, ProjectVariables should be keyed by key+environmentScope.

This PR now allows for the provider to be keyed by key+environmentScope.

The current implementation is a workaround for a bug within Gitlab API.
https://gitlab.com/gitlab-org/gitlab/-/issues/20661

Brute-force approach to workaround the shortcoming of the Gitlab API issue:

1. Fetch all ProjectVariables
2. Filter ProjectVariables for the key that is being updated or deleted
3. Issue n Delete api calls for ProjectVariable key where n is the number of ProjectVariables matching the key
4. Re-create without change ProjectVariables for key & environmentScope that are not being updated/deleted
5. If Update, re-create with change ProjectVariable for key & environmentScope that is being updated
6. If Delete, no API call for the ProjectVariable for key & environmentScope that is should be deleted

Fixes #213
Fixes #298
Fixes #235

[techthumb]

@roidelapluie Could you please provide feedback to get this merged into master?

[techthumb]

@ringods Could you please provide feedback to get this merged into master?

[Nosmoht]

Hi all,

i've just tested the branch with some existing Gitlab project environment scoped variables and got following error while terraform refresh:

...
Error: Unexpected ID format ("project-id:key"). Expected project:key:env
...

Is there anything planed on how to transform existing variables with old structure to new one?

Should we?

• Create new variables with ID format project:key:env
• Update existing variable with format project:key to project:key:env with env default * All (Default)
• Use env * All (Default) as the default

[techthumb]

Is there anything planed on how to transform existing variables with old structure to new one?

I didn't code for this.
I did recently learn about StateUpgrader & will consider using it here.

Should we?

• Create new variables with ID format project:key:env
• Update existing variable with format project:key to project:key:env with env default * All (Default)
• Use env * All (Default) as the default

I don't know the correct answer to 👆, because there is a matrix of non-trivial cases to consider here:

1. One-to-one mapping between project:key and env
2. One-to-many mapping between project:key and env
3. Is a user requested new change being applied?

Based on my (limited) understanding of the TF plugin api, and the missing information in the state file, I suspect that we can't answer these questions to automate a fix.

@Nosmoht Can you validate my thinking here?
There is a good chance that I may be missing some concepts that may provide an automated solution.

[Nosmoht]

Fair question, which i'm unable to answer. Can we get someone from Gitlab into this?

[woz5999]

@Nosmoht here's the relevant gitlab issue. it looks like they're about to release an api update shortly: https://gitlab.com/gitlab-org/gitlab/-/issues/9912

I imagine once that's live the provider will need to be updated to support the new changes.

[enkov]

Any news here? Gitlab released a fix for environment variables.

[thapakazi]

If we are good, lets merge this, we couldn't update our CI variables because of this 😞

[woz5999]

This probably shouldn't get merged unless and it'll it's checked against the gitlab api changes released recently to address the underlying api issue. people in #213 are saying this resource is currently broke entirely for them.

[techthumb]

Hi @nicholasklick,

Thanks for looking into this.

As mentioned in the issue description, this implementation applies a brute force approach.

Given that the api now supports filtering by environment scope, this implementation is perhaps invalid.

[nicholasklick]

@techthumb good to know. @sfang97 will be investigating this further and help moving this toward merge.

[sfang97]

@techthumb Thanks for your work on this, a lot of people are looking forward to this being merged! 😄
Since GitLab has fixed the bug in the API, would you be willing to modify this PR to use the fix? The API implementation is not in go-gitlab yet, but perhaps @remil1000's solution here: xanzy/go-gitlab#746 (comment) could be used to simplify this PR?
Environment scope bug fix MR: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/34490
^ That filter is currently behind a feature flag, but when 13.4 is released on 2020-08-22 it will be enabled by default: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/39209

[techthumb]

The scope of this issue is getting large:

1. migrate state file
2. detect feature flag
3. implementation based on the value of the feature flag
4. testing with feature flag on and off

@sfang97 I don't have the bandwidth ATM to work on this.
I am happy to review and manually test changes though.

[sfang97]

@roidelapluie Could you take a look at this Julien? The current implementation is a brute force approach, which may be a sufficient first iteration to fix the problems people are having. WDYT?

[roidelapluie]

I am not using that gitlab feature, so I do not know if that really helps or if we want a full solution from the start.

[armsnyder]

If you can it would be good to include a new test that creates two different variables using the same key but different scopes. That would verify the new behavior, and the bug reported in #396 in particular.

[nicholasklick]

@armsnyder @sfang97 how can we work to move this through?

1. It seems like the original committer doesnt have time to complete updating the PR
2. Can we better leverage go-gitlab as mentioned here to simplify this PR?

[armsnyder]

I agree with @sfang97 that the current hacky solution of iterating over all variables is good enough. The thing that's missing for me is validating that the provider version upgrade is safe. Changing a resource's ID format is listed as a potentially breaking change by Terraform. It isn't clear to me from Terraform's docs if implementing state migration makes it non-breaking, or if Terraform still wants it to be a major version upgrade. Does anyone know?

[roidelapluie]

Anyway major version upgrade is not an issue. We can release a 3.0.0 to be safe.

[nicholasklick]

@armsnyder @roidelapluie @sfang97 I suggest that we merge this and bump to a major version update.
This seems to be blocking a lot of people.
Feel free to do it.
We can iterate and improve this going forward.

[nicholasklick]

Seems like there are conflicts now.

[armsnyder]

I can take a stab at a new PR, unless @sfang97 or someone else has already started on one.

[sfang97]

@armsnyder Go for it! Thanks for working on this :)

[abelmokadem]

Is it possible to get the GitLab provider from this PR and run it in my own project?

[armsnyder]

@abelmokadem It is possible. You can clone the fork from this PR and build the provider yourself with make build, then install the binary as a third party Terrraform plugin. Beware that there is the potential for a breaking change for you when you switch back to an official release of this provider, after we get a fix in and released.

[armsnyder]

Closing this since we merged #409