Merge pull request #998 from Shocktrooper/lint-issue-AT001

Lint Issues- Testing out addition of Check Destroy in some Acceptance Tests + other refactoring

internal/provider/resource_gitlab_group.go

@@ -4,7 +4,6 @@ import (
 	"context"
 	"fmt"
 	"log"
-	"net/http"
 	"strings"
 	"time"
 
@@ -251,9 +250,9 @@ func resourceGitlabGroupRead(ctx context.Context, d *schema.ResourceData, meta i
 	client := meta.(*gitlab.Client)
 	log.Printf("[DEBUG] read gitlab group %s", d.Id())
 
-	group, resp, err := client.Groups.GetGroup(d.Id(), nil, gitlab.WithContext(ctx))
+	group, _, err := client.Groups.GetGroup(d.Id(), nil, gitlab.WithContext(ctx))
 	if err != nil {
-		if resp != nil && resp.StatusCode == http.StatusNotFound {
+		if is404(err) {
 			log.Printf("[DEBUG] gitlab group %s not found so removing from state", d.Id())
 			d.SetId("")
 			return nil

internal/provider/resource_gitlab_group_share_group.go

@@ -4,7 +4,6 @@ import (
 	"context"
 	"fmt"
 	"log"
-	"net/http"
 	"strconv"
 
 	"github.com/hashicorp/terraform-plugin-sdk/v2/diag"
@@ -93,9 +92,9 @@ func resourceGitlabGroupShareGroupRead(ctx context.Context, d *schema.ResourceDa
 	}
 
 	// Query main group
-	group, resp, err := client.Groups.GetGroup(groupId, nil, gitlab.WithContext(ctx))
+	group, _, err := client.Groups.GetGroup(groupId, nil, gitlab.WithContext(ctx))
 	if err != nil {
-		if resp != nil && resp.StatusCode == http.StatusNotFound {
+		if is404(err) {
 			log.Printf("[DEBUG] gitlab group %s not found so removing from state", groupId)
 			d.SetId("")
 			return nil

internal/provider/resource_gitlab_group_share_group_test.go

@@ -4,64 +4,58 @@ import (
 	"fmt"
 	"testing"
 
-	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/acctest"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/terraform"
 	"github.com/xanzy/go-gitlab"
 )
 
 func TestAccGitlabGroupShareGroup_basic(t *testing.T) {
-	randName := acctest.RandomWithPrefix("acctest")
+	testAccCheck(t)
+
+	mainGroup := testAccCreateGroups(t, 1)[0]
+	sharedGroup := testAccCreateGroups(t, 1)[0]
 
-	// lintignore: AT001 // TODO: Resolve this tfproviderlint issue
 	resource.Test(t, resource.TestCase{
 		PreCheck:          func() { testAccPreCheck(t) },
 		ProviderFactories: providerFactories,
+		CheckDestroy:      testAccCheckGitlabShareGroupDestroy,
 		Steps: []resource.TestStep{
 			// Share a new group with another group
 			{
-				Config: testAccGitlabGroupShareGroupConfig(
-					randName,
+				Config: testAccGitlabGroupShareGroupConfig(mainGroup.ID, sharedGroup.ID,
 					`
 					group_access 	 = "guest"
 					expires_at     = "2099-01-01"
 					`,
 				),
-				Check: testAccCheckGitlabGroupSharedWithGroup(randName, "2099-01-01", gitlab.GuestPermissions),
+				Check: testAccCheckGitlabGroupSharedWithGroup(mainGroup.Name, sharedGroup.Name, "2099-01-01", gitlab.GuestPermissions),
+			},
+			{
+				// Verify Import
+				ResourceName:      "gitlab_group_share_group.test",
+				ImportState:       true,
+				ImportStateVerify: true,
 			},
 			// Update the share group
 			{
-				Config: testAccGitlabGroupShareGroupConfig(randName, `group_access = "reporter"`),
-				Check:  testAccCheckGitlabGroupSharedWithGroup(randName, "", gitlab.ReporterPermissions),
+				Config: testAccGitlabGroupShareGroupConfig(mainGroup.ID, sharedGroup.ID, `group_access = "reporter"`),
+				Check:  testAccCheckGitlabGroupSharedWithGroup(mainGroup.Name, sharedGroup.Name, "", gitlab.ReporterPermissions),
 			},
-			// Delete the gitlab_group_share_group resource
 			{
-				Config: testAccGitlabGroupShareGroupConfigDelete(randName),
-				Check:  testAccCheckGitlabGroupIsNotShared(randName),
+				// Verify Import
+				ResourceName:      "gitlab_group_share_group.test",
+				ImportState:       true,
+				ImportStateVerify: true,
 			},
-		},
-	})
-}
-
-// lintignore: AT002 // TODO: Resolve this tfproviderlint issue
-func TestAccGitlabGroupShareGroup_import(t *testing.T) {
-	randName := acctest.RandomWithPrefix("acctest")
-
-	resource.Test(t, resource.TestCase{
-		ProviderFactories: providerFactories,
-		PreCheck:          func() { testAccPreCheck(t) },
-		CheckDestroy:      testAccCheckGitlabGroupDestroy,
-		Steps: []resource.TestStep{
+			// Update share group back to initial settings
 			{
-				// create shared groups
-				Config: testAccGitlabGroupShareGroupConfig(
-					randName,
+				Config: testAccGitlabGroupShareGroupConfig(mainGroup.ID, sharedGroup.ID,
 					`
 					group_access 	 = "guest"
-					expires_at     = "2099-03-03"
+					expires_at     = "2099-01-01"
 					`,
 				),
-				Check: testAccCheckGitlabGroupSharedWithGroup(randName, "2099-03-03", gitlab.GuestPermissions),
+				Check: testAccCheckGitlabGroupSharedWithGroup(mainGroup.Name, sharedGroup.Name, "2099-01-01", gitlab.GuestPermissions),
 			},
 			{
 				// Verify Import
@@ -73,13 +67,9 @@ func TestAccGitlabGroupShareGroup_import(t *testing.T) {
 	})
 }
 
-func testAccCheckGitlabGroupSharedWithGroup(
-	groupName string,
-	expireTime string,
-	accessLevel gitlab.AccessLevelValue,
-) resource.TestCheckFunc {
+func testAccCheckGitlabGroupSharedWithGroup(mainGroupName string, sharedGroupName string, expireTime string, accessLevel gitlab.AccessLevelValue) resource.TestCheckFunc {
 	return func(_ *terraform.State) error {
-		mainGroup, _, err := testGitlabClient.Groups.GetGroup(fmt.Sprintf("%s_main", groupName), nil)
+		mainGroup, _, err := testGitlabClient.Groups.GetGroup(mainGroupName, nil)
 		if err != nil {
 			return err
 		}
@@ -91,8 +81,8 @@ func testAccCheckGitlabGroupSharedWithGroup(
 
 		sharedGroup := mainGroup.SharedWithGroups[0]
 
-		if sharedGroup.GroupName != fmt.Sprintf("%s_share", groupName) {
-			return fmt.Errorf("group name was %s (wanted %s)", sharedGroup.GroupName, fmt.Sprintf("%s_share", groupName))
+		if sharedGroup.GroupName != sharedGroupName {
+			return fmt.Errorf("group name was %s (wanted %s)", sharedGroup.GroupName, sharedGroupName)
 		}
 
 		if gitlab.AccessLevelValue(sharedGroup.GroupAccessLevel) != accessLevel {
@@ -109,62 +99,47 @@ func testAccCheckGitlabGroupSharedWithGroup(
 	}
 }
 
-func testAccCheckGitlabGroupIsNotShared(groupName string) resource.TestCheckFunc {
-	return func(_ *terraform.State) error {
-		mainGroup, _, err := testGitlabClient.Groups.GetGroup(fmt.Sprintf("%s_main", groupName), nil)
-		if err != nil {
-			return err
-		}
-
-		sharedGroupsCount := len(mainGroup.SharedWithGroups)
-		if sharedGroupsCount != 0 {
-			return fmt.Errorf("Number of shared groups was %d (wanted %d)", sharedGroupsCount, 0)
+func testAccCheckGitlabShareGroupDestroy(s *terraform.State) error {
+	var groupId string
+	var sharedGroupId int
+	var err error
+
+	for _, rs := range s.RootModule().Resources {
+		if rs.Type == "gitlab_group_share_group" {
+			groupId, sharedGroupId, err = groupIdsFromId(rs.Primary.ID)
+			if err != nil {
+				return fmt.Errorf("[ERROR] cannot get Group ID and ShareGroupId from input: %v", rs.Primary.ID)
+			}
+
+			// Get Main Group
+			group, _, err := testGitlabClient.Groups.GetGroup(groupId, nil)
+			if err != nil {
+				return err
+			}
+
+			// Make sure that SharedWithGroups attribute on the main group does not contain the shared group id at all
+			for _, sharedGroup := range group.SharedWithGroups {
+				if sharedGroupId == sharedGroup.GroupID {
+					return fmt.Errorf("GitLab Group Share %d still exists", sharedGroupId)
+				}
+			}
 		}
-
-		return nil
 	}
+
+	return nil
 }
 
-func testAccGitlabGroupShareGroupConfig(
-	randName string,
-	shareGroupSettings string,
-) string {
+func testAccGitlabGroupShareGroupConfig(mainGroupId int, shareGroupId int, shareGroupSettings string) string {
 	return fmt.Sprintf(
 		`
-		resource "gitlab_group" "test_main" {
-		  name = "%[1]s_main"
-		  path = "%[1]s_main"
-		}
-
-		resource "gitlab_group" "test_share" {
-		  name = "%[1]s_share"
-		  path = "%[1]s_share"
-		}
-
 		resource "gitlab_group_share_group" "test" {
-		  group_id       = gitlab_group.test_main.id
-			share_group_id = gitlab_group.test_share.id
-			%[2]s
+		  group_id       = %[1]d
+			share_group_id = %[2]d
+			%[3]s
 		}
 		`,
-		randName,
+		mainGroupId,
+		shareGroupId,
 		shareGroupSettings,
 	)
 }
-
-func testAccGitlabGroupShareGroupConfigDelete(randName string) string {
-	return fmt.Sprintf(
-		`
-		resource "gitlab_group" "test_main" {
-		  name = "%[1]s_main"
-		  path = "%[1]s_main"
-		}
-
-		resource "gitlab_group" "test_share" {
-		  name = "%[1]s_share"
-		  path = "%[1]s_share"
-		}
-		`,
-		randName,
-	)
-}

internal/provider/resource_gitlab_group_test.go

@@ -2,7 +2,6 @@ package provider
 
 import (
 	"fmt"
-	"net/http"
 	"testing"
 	"time"
 
@@ -284,8 +283,8 @@ func testAccCheckGitlabGroupDisappears(group *gitlab.Group) resource.TestCheckFu
 		// Fixes groups API async deletion issue
 		// https://github.com/gitlabhq/terraform-provider-gitlab/issues/319
 		for start := time.Now(); time.Since(start) < 15*time.Second; {
-			g, resp, err := testGitlabClient.Groups.GetGroup(group.ID, nil)
-			if resp != nil && resp.StatusCode == http.StatusNotFound {
+			g, _, err := testGitlabClient.Groups.GetGroup(group.ID, nil)
+			if is404(err) {
 				return nil
 			}
 			if g != nil && g.MarkedForDeletionOn != nil {

internal/provider/resource_gitlab_instance_variable.go

@@ -3,7 +3,6 @@ package provider
 import (
 	"context"
 	"log"
-	"net/http"
 
 	"github.com/hashicorp/terraform-plugin-sdk/v2/diag"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
@@ -62,9 +61,9 @@ func resourceGitlabInstanceVariableRead(ctx context.Context, d *schema.ResourceD
 
 	log.Printf("[DEBUG] read gitlab instance level CI variable %s", key)
 
-	v, resp, err := client.InstanceVariables.GetVariable(key, gitlab.WithContext(ctx))
+	v, _, err := client.InstanceVariables.GetVariable(key, gitlab.WithContext(ctx))
 	if err != nil {
-		if resp.StatusCode == http.StatusNotFound {
+		if is404(err) {
 			log.Printf("[DEBUG] gitlab instance level CI variable for %s not found so removing from state", d.Id())
 			d.SetId("")
 			return nil

internal/provider/resource_gitlab_instance_variable_test.go

@@ -15,10 +15,10 @@ func TestAccGitlabInstanceVariable_basic(t *testing.T) {
 	var instanceVariable gitlab.InstanceVariable
 	rString := acctest.RandString(5)
 
-	// lintignore: AT001 // TODO: Resolve this tfproviderlint issue
 	resource.Test(t, resource.TestCase{
 		PreCheck:          func() { testAccPreCheck(t) },
 		ProviderFactories: providerFactories,
+		CheckDestroy:      testAccCheckGitlabInstanceVariableDestroy,
 		Steps: []resource.TestStep{
 			// Create a variable with default options
 			{
@@ -120,6 +120,29 @@ func testAccCheckGitlabInstanceVariableExists(n string, instanceVariable *gitlab
 	}
 }
 
+func testAccCheckGitlabInstanceVariableDestroy(s *terraform.State) error {
+	var key string
+
+	for _, rs := range s.RootModule().Resources {
+		if rs.Type == "gitlab_instance_variable" {
+			key = rs.Primary.ID
+		}
+	}
+
+	iv, _, err := testGitlabClient.InstanceVariables.GetVariable(key)
+	if err == nil {
+		if iv != nil {
+			return fmt.Errorf("Instance Variable %s still exists", key)
+		}
+	} else {
+		if !is404(err) {
+			return err
+		}
+	}
+
+	return nil
+}
+
 type testAccGitlabInstanceVariableExpectedAttributes struct {
 	Key       string
 	Value     string

internal/provider/resource_gitlab_project_freeze_period.go

@@ -4,7 +4,6 @@ import (
 	"context"
 	"fmt"
 	"log"
-	"net/http"
 	"strconv"
 
 	"github.com/hashicorp/terraform-plugin-sdk/v2/diag"
@@ -84,9 +83,9 @@ func resourceGitlabProjectFreezePeriodRead(ctx context.Context, d *schema.Resour
 
 	log.Printf("[DEBUG] read gitlab FreezePeriod %s/%d", projectID, freezePeriodID)
 
-	freezePeriod, resp, err := client.FreezePeriods.GetFreezePeriod(projectID, freezePeriodID, gitlab.WithContext(ctx))
+	freezePeriod, _, err := client.FreezePeriods.GetFreezePeriod(projectID, freezePeriodID, gitlab.WithContext(ctx))
 	if err != nil {
-		if resp != nil && resp.StatusCode == http.StatusNotFound {
+		if is404(err) {
 			log.Printf("[DEBUG] project freeze period for %s not found so removing it from state", d.Id())
 			d.SetId("")
 			return nil

internal/provider/resource_gitlab_project_membership.go

@@ -4,7 +4,6 @@ import (
 	"context"
 	"fmt"
 	"log"
-	"net/http"
 	"strconv"
 	"strings"
 
@@ -85,9 +84,9 @@ func resourceGitlabProjectMembershipRead(ctx context.Context, d *schema.Resource
 		return diag.FromErr(err)
 	}
 
-	projectMember, resp, err := client.ProjectMembers.GetProjectMember(projectId, userId, gitlab.WithContext(ctx))
+	projectMember, _, err := client.ProjectMembers.GetProjectMember(projectId, userId, gitlab.WithContext(ctx))
 	if err != nil {
-		if resp != nil && resp.StatusCode == http.StatusNotFound {
+		if is404(err) {
 			log.Printf("[DEBUG] gitlab project membership for %s not found so removing from state", d.Id())
 			d.SetId("")
 			return nil