Add failing group variable update test to test issue #861

internal/provider/environment_scope_helper.go

@@ -0,0 +1,26 @@
+package provider
+
+import (
+	"context"
+	"net/url"
+
+	"github.com/hashicorp/go-retryablehttp"
+	"github.com/xanzy/go-gitlab"
+)
+
+// withEnvironmentScopeFilter adds the environment scope filter query parameter to the URL.
+// This function is supposed to be used as `gitlab.RequestOptionFunc` parameter.
+// The parameter is documented in the upstream GitLab API docs:
+// https://docs.gitlab.com/ee/api/project_level_variables.html#the-filter-parameter
+func withEnvironmentScopeFilter(ctx context.Context, environmentScope string) gitlab.RequestOptionFunc {
+	return func(req *retryablehttp.Request) error {
+		*req = *req.WithContext(ctx)
+		query, err := url.ParseQuery(req.Request.URL.RawQuery)
+		if err != nil {
+			return err
+		}
+		query.Set("filter[environment_scope]", environmentScope)
+		req.Request.URL.RawQuery = query.Encode()
+		return nil
+	}
+}

internal/provider/resource_gitlab_group_variable.go

@@ -6,27 +6,11 @@ import (
 	"log"
 	"strings"
 
-	"github.com/hashicorp/go-retryablehttp"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/diag"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
 	gitlab "github.com/xanzy/go-gitlab"
 )
 
-// modifyRequestAddEnvironmentFilter returns a RequestOptionFunc function that
-// can be passed to the go-gitlab library calls to add the environment scope to
-// requests to lookup, modification, and deletion requests. Since gitlab 13.11,
-// an environment variable key is no longer unique and is composit-keyed with
-// the scope.
-// See https://docs.gitlab.com/ee/ci/variables/#add-a-cicd-variable-to-a-group
-func modifyRequestAddEnvironmentFilter(scope string) gitlab.RequestOptionFunc {
-	return func(r *retryablehttp.Request) error {
-		queryParams := r.URL.Query()
-		queryParams.Add("filter[environment_scope]", scope)
-		r.URL.RawQuery = queryParams.Encode()
-		return nil
-	}
-}
-
 var _ = registerResource("gitlab_group_variable", func() *schema.Resource {
 	return &schema.Resource{
 		Description: "This resource allows you to create and manage CI/CD variables for your GitLab groups.\n" +
@@ -145,7 +129,7 @@ func resourceGitlabGroupVariableRead(ctx context.Context, d *schema.ResourceData
 		group,
 		key,
 		gitlab.WithContext(ctx),
-		modifyRequestAddEnvironmentFilter(scope),
+		withEnvironmentScopeFilter(ctx, scope),
 	)
 	if err != nil {
 		if is404(err) {
@@ -191,7 +175,7 @@ func resourceGitlabGroupVariableUpdate(ctx context.Context, d *schema.ResourceDa
 		key,
 		options,
 		gitlab.WithContext(ctx),
-		modifyRequestAddEnvironmentFilter(environmentScope),
+		withEnvironmentScopeFilter(ctx, environmentScope),
 	)
 	if err != nil {
 		return diag.FromErr(err)
@@ -210,7 +194,7 @@ func resourceGitlabGroupVariableDelete(ctx context.Context, d *schema.ResourceDa
 		group,
 		key,
 		gitlab.WithContext(ctx),
-		modifyRequestAddEnvironmentFilter(environmentScope),
+		withEnvironmentScopeFilter(ctx, environmentScope),
 	)
 	if err != nil {
 		return diag.FromErr(err)

internal/provider/resource_gitlab_group_variable_test.go

@@ -1,6 +1,7 @@
 package provider
 
 import (
+	"context"
 	"fmt"
 	"testing"
 
@@ -65,64 +66,87 @@ func TestAccGitlabGroupVariable_scope(t *testing.T) {
 	var groupVariableA, groupVariableB gitlab.GroupVariable
 	rString := acctest.RandString(5)
 
+	defaultValueA := fmt.Sprintf("value-%s-a", rString)
+	defaultValueB := fmt.Sprintf("value-%s-b", rString)
 	resource.Test(t, resource.TestCase{
 		PreCheck:          func() { testAccPreCheck(t) },
 		ProviderFactories: providerFactories,
 		CheckDestroy:      testAccCheckGitlabGroupVariableDestroy,
 		Steps: []resource.TestStep{
 			// Create a group and variables with same keys, different scopes
 			{
-				Config:   testAccGitlabGroupVariableScopeConfig(rString, "*", "review/*"),
+				Config:   testAccGitlabGroupVariableScopeConfig(rString, "*", "review/*", defaultValueA, defaultValueB),
 				SkipFunc: isRunningInCE,
 				Check: resource.ComposeTestCheckFunc(
 					testAccCheckGitlabGroupVariableExists("gitlab_group_variable.a", &groupVariableA),
 					testAccCheckGitlabGroupVariableExists("gitlab_group_variable.b", &groupVariableB),
 					testAccCheckGitlabGroupVariableAttributes(&groupVariableA, &testAccGitlabGroupVariableExpectedAttributes{
 						Key:              fmt.Sprintf("key_%s", rString),
-						Value:            fmt.Sprintf("value-%s-a", rString),
+						Value:            defaultValueA,
 						EnvironmentScope: "*",
 					}),
 					testAccCheckGitlabGroupVariableAttributes(&groupVariableB, &testAccGitlabGroupVariableExpectedAttributes{
 						Key:              fmt.Sprintf("key_%s", rString),
-						Value:            fmt.Sprintf("value-%s-b", rString),
+						Value:            defaultValueB,
 						EnvironmentScope: "review/*",
 					}),
 				),
 			},
 			// Change a variable's scope
 			{
-				Config:   testAccGitlabGroupVariableScopeConfig(rString, "my-new-scope", "review/*"),
+				Config:   testAccGitlabGroupVariableScopeConfig(rString, "my-new-scope", "review/*", defaultValueA, defaultValueB),
 				SkipFunc: isRunningInCE,
 				Check: resource.ComposeTestCheckFunc(
 					testAccCheckGitlabGroupVariableExists("gitlab_group_variable.a", &groupVariableA),
 					testAccCheckGitlabGroupVariableExists("gitlab_group_variable.b", &groupVariableB),
 					testAccCheckGitlabGroupVariableAttributes(&groupVariableA, &testAccGitlabGroupVariableExpectedAttributes{
 						Key:              fmt.Sprintf("key_%s", rString),
-						Value:            fmt.Sprintf("value-%s-a", rString),
+						Value:            defaultValueA,
 						EnvironmentScope: "my-new-scope",
 					}),
 					testAccCheckGitlabGroupVariableAttributes(&groupVariableB, &testAccGitlabGroupVariableExpectedAttributes{
 						Key:              fmt.Sprintf("key_%s", rString),
-						Value:            fmt.Sprintf("value-%s-b", rString),
+						Value:            defaultValueB,
 						EnvironmentScope: "review/*",
 					}),
 				),
 			},
 			// Change both variables scopes at the same time
 			{
-				Config:   testAccGitlabGroupVariableScopeConfig(rString, "my-new-new-scope", "review/hello-world"),
+				Config:   testAccGitlabGroupVariableScopeConfig(rString, "my-new-new-scope", "review/hello-world", defaultValueA, defaultValueB),
 				SkipFunc: isRunningInCE,
 				Check: resource.ComposeTestCheckFunc(
 					testAccCheckGitlabGroupVariableExists("gitlab_group_variable.a", &groupVariableA),
 					testAccCheckGitlabGroupVariableExists("gitlab_group_variable.b", &groupVariableB),
 					testAccCheckGitlabGroupVariableAttributes(&groupVariableA, &testAccGitlabGroupVariableExpectedAttributes{
 						Key:              fmt.Sprintf("key_%s", rString),
-						Value:            fmt.Sprintf("value-%s-a", rString),
+						Value:            defaultValueA,
 						EnvironmentScope: "my-new-new-scope",
 					}),
 					testAccCheckGitlabGroupVariableAttributes(&groupVariableB, &testAccGitlabGroupVariableExpectedAttributes{
 						Key:              fmt.Sprintf("key_%s", rString),
-						Value:            fmt.Sprintf("value-%s-b", rString),
+						Value:            defaultValueB,
+						EnvironmentScope: "review/hello-world",
+					}),
+				),
+			},
+			// Change value of one variable
+			{
+				Config: testAccGitlabGroupVariableScopeConfig(rString, "my-new-new-scope", "review/hello-world", defaultValueA, fmt.Sprintf("new-value-for-b-%s", rString)),
+				// SkipFunc: isRunningInCE,
+				// NOTE(TF): this test sporadically fails because of this: https://gitlab.com/gitlab-org/gitlab/-/issues/333296
+				SkipFunc: func() (bool, error) { return true, nil },
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckGitlabGroupVariableExists("gitlab_group_variable.a", &groupVariableA),
+					testAccCheckGitlabGroupVariableExists("gitlab_group_variable.b", &groupVariableB),
+					testAccCheckGitlabGroupVariableAttributes(&groupVariableA, &testAccGitlabGroupVariableExpectedAttributes{
+						Key:              fmt.Sprintf("key_%s", rString),
+						Value:            defaultValueA,
+						EnvironmentScope: "my-new-new-scope",
+					}),
+					testAccCheckGitlabGroupVariableAttributes(&groupVariableB, &testAccGitlabGroupVariableExpectedAttributes{
+						Key:              fmt.Sprintf("key_%s", rString),
+						Value:            fmt.Sprintf("new-value-for-b-%s", rString),
 						EnvironmentScope: "review/hello-world",
 					}),
 				),
@@ -146,7 +170,7 @@ func testAccCheckGitlabGroupVariableExists(n string, groupVariable *gitlab.Group
 		if key == "" {
 			return fmt.Errorf("No variable key is set")
 		}
-		gotVariable, _, err := testGitlabClient.GroupVariables.GetVariable(repoName, key, modifyRequestAddEnvironmentFilter(rs.Primary.Attributes["environment_scope"]))
+		gotVariable, _, err := testGitlabClient.GroupVariables.GetVariable(repoName, key, withEnvironmentScopeFilter(context.Background(), rs.Primary.Attributes["environment_scope"]))
 		if err != nil {
 			return err
 		}
@@ -245,7 +269,7 @@ resource "gitlab_group_variable" "foo" {
 	`, rString, rString, rString, rString)
 }
 
-func testAccGitlabGroupVariableScopeConfig(rString, scopeA, scopeB string) string {
+func testAccGitlabGroupVariableScopeConfig(rString, scopeA, scopeB string, valueA, valueB string) string {
 	return fmt.Sprintf(`
 resource "gitlab_group" "foo" {
   name = "foo%v"
@@ -255,15 +279,15 @@ resource "gitlab_group" "foo" {
 resource "gitlab_group_variable" "a" {
   group             = "${gitlab_group.foo.id}"
   key               = "key_%s"
-  value             = "value-%s-a"
+  value             = "%s"
   environment_scope = "%s"
 }
 
 resource "gitlab_group_variable" "b" {
   group             = "${gitlab_group.foo.id}"
   key               = "key_%s"
-  value             = "value-%s-b"
+  value             = "%s"
   environment_scope = "%s"
 }
-	`, rString, rString, rString, rString, scopeA, rString, rString, scopeB)
+	`, rString, rString, rString, valueA, scopeA, rString, valueB, scopeB)
 }

internal/provider/resource_gitlab_project_variable.go

@@ -5,10 +5,8 @@ import (
 	"errors"
 	"log"
 	"net/http"
-	"net/url"
 	"strings"
 
-	retryablehttp "github.com/hashicorp/go-retryablehttp"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/diag"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
 	gitlab "github.com/xanzy/go-gitlab"
@@ -230,19 +228,6 @@ func isInvalidValueError(err error) bool {
 		strings.Contains(httpErr.Message, "invalid")
 }
 
-func withEnvironmentScopeFilter(ctx context.Context, environmentScope string) gitlab.RequestOptionFunc {
-	return func(req *retryablehttp.Request) error {
-		*req = *req.WithContext(ctx)
-		query, err := url.ParseQuery(req.Request.URL.RawQuery)
-		if err != nil {
-			return err
-		}
-		query.Set("filter[environment_scope]", environmentScope)
-		req.Request.URL.RawQuery = query.Encode()
-		return nil
-	}
-}
-
 var errProjectVariableNotExist = errors.New("project variable does not exist")
 
 func getProjectVariable(ctx context.Context, client *gitlab.Client, project interface{}, key, environmentScope string) (*gitlab.ProjectVariable, error) {