Revert "SUP-1402: Agent token resource retries (#382)"

This reverts commit deb70c3.
buildkite · Aug 30, 2023 · a254414 · a254414
1 parent deb70c3
commit a254414
Show file tree

Hide file tree

Showing 3 changed files with 98 additions and 214 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -6,8 +6,7 @@ All notable changes to this project will be documented in this file.
 
 - SUP-1392: Random test suite names in tests/t.Run() conversion [[PR #376](https://github.com/buildkite/terraform-provider-buildkite/pull/376)] @james2791
 - SUP-1374 Add timeout to provider and cluster datasource [[PR #363](https://github.com/buildkite/terraform-provider-buildkite/pull/363)] @jradtilbrook
-- SUP-1374 Remove timeout context [[PR #378](https://github.com/buildkite/terraform-provider-buildkite/pull/378)] @jradtilbrook
-- SUP-1402: Agent token resource retries [[PR #382](https://github.com/buildkite/terraform-provider-buildkite/pull/382)] @james2791
+    - SUP-1374 Remove timeout context [[PR #378](https://github.com/buildkite/terraform-provider-buildkite/pull/378)] @jradtilbrook
 
 ### Changes
 

diff --git a/buildkite/resource_agent_token.go b/buildkite/resource_agent_token.go
@@ -9,7 +9,6 @@ import (
 	"github.com/hashicorp/terraform-plugin-framework/resource/schema/planmodifier"
 	"github.com/hashicorp/terraform-plugin-framework/resource/schema/stringplanmodifier"
 	"github.com/hashicorp/terraform-plugin-framework/types"
-	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/retry"
 	"github.com/shurcooL/graphql"
 )
 
@@ -46,95 +45,34 @@ func (at *AgentTokenResource) Configure(ctx context.Context, req resource.Config
 
 func (at *AgentTokenResource) Create(ctx context.Context, req resource.CreateRequest, resp *resource.CreateResponse) {
 	var plan, state AgentTokenStateModel
+	resp.Diagnostics.Append(req.Plan.Get(ctx, &plan)...)
 
-	diags := req.Plan.Get(ctx, &plan)
-	resp.Diagnostics.Append(diags...)
-
-	if resp.Diagnostics.HasError() {
-		return
-	}
-
-	timeout, diags := at.client.timeouts.Create(ctx, DefaultTimeout)
-	resp.Diagnostics.Append(diags...)
-
-	if resp.Diagnostics.HasError() {
-		return
-	}
-
-	var r *createAgentTokenResponse
-	err := retry.RetryContext(ctx, timeout, func() *retry.RetryError {
-		var err error
-		r, err = createAgentToken(ctx,
-			at.client.genqlient,
-			at.client.organizationId,
-			plan.Description.ValueStringPointer(),
-		)
-
-		if err != nil {
-			if isRetryableError(err) {
-				return retry.RetryableError(err)
-			}
-			return retry.NonRetryableError(err)
-		}
-
-		return nil
-	})
+	apiResponse, err := createAgentToken(ctx,
+		at.client.genqlient,
+		at.client.organizationId,
+		plan.Description.ValueStringPointer(),
+	)
 
 	if err != nil {
-		resp.Diagnostics.AddError(
-			"Unable to create agent token",
-			fmt.Sprintf("Unable to create agent token: %s", err.Error()),
-		)
-		return
+		resp.Diagnostics.AddError(err.Error(), err.Error())
 	}
 
-	state.Description = types.StringPointerValue(r.AgentTokenCreate.AgentTokenEdge.Node.Description)
-	state.Id = types.StringValue(r.AgentTokenCreate.AgentTokenEdge.Node.Id)
-	state.Token = types.StringValue(r.AgentTokenCreate.TokenValue)
-	state.Uuid = types.StringValue(r.AgentTokenCreate.AgentTokenEdge.Node.Uuid)
+	state.Description = types.StringPointerValue(apiResponse.AgentTokenCreate.AgentTokenEdge.Node.Description)
+	state.Id = types.StringValue(apiResponse.AgentTokenCreate.AgentTokenEdge.Node.Id)
+	state.Token = types.StringValue(apiResponse.AgentTokenCreate.TokenValue)
+	state.Uuid = types.StringValue(apiResponse.AgentTokenCreate.AgentTokenEdge.Node.Uuid)
 
 	resp.Diagnostics.Append(resp.State.Set(ctx, &state)...)
 }
 
 func (at *AgentTokenResource) Delete(ctx context.Context, req resource.DeleteRequest, resp *resource.DeleteResponse) {
-	var state AgentTokenStateModel
-
-	diags := req.State.Get(ctx, &state)
-	resp.Diagnostics.Append(diags...)
+	var plan AgentTokenStateModel
+	resp.Diagnostics.Append(req.State.Get(ctx, &plan)...)
 
-	if resp.Diagnostics.HasError() {
-		return
-	}
-
-	timeout, diags := at.client.timeouts.Delete(ctx, DefaultTimeout)
-	resp.Diagnostics.Append(diags...)
-
-	if resp.Diagnostics.HasError() {
-		return
-	}
-
-	err := retry.RetryContext(ctx, timeout, func() *retry.RetryError {
-		_, err := revokeAgentToken(ctx,
-			at.client.genqlient,
-			state.Id.ValueString(),
-			"Revoked by Terraform",
-		)
-
-		if err != nil {
-			if isRetryableError(err) {
-				return retry.RetryableError(err)
-			}
-			return retry.NonRetryableError(err)
-		}
-		return nil
-	})
+	_, err := revokeAgentToken(ctx, at.client.genqlient, plan.Id.ValueString(), "Revoked by Terraform")
 
 	if err != nil {
-		resp.Diagnostics.AddError(
-			"Unable to revoke agent token",
-			fmt.Sprintf("Unable to revoke agent token: %s", err.Error()),
-		)
-		return
+		resp.Diagnostics.AddError(err.Error(), err.Error())
 	}
 }
 
@@ -144,56 +82,23 @@ func (AgentTokenResource) Metadata(ctx context.Context, req resource.MetadataReq
 
 func (at *AgentTokenResource) Read(ctx context.Context, req resource.ReadRequest, resp *resource.ReadResponse) {
 	var plan, state AgentTokenStateModel
+	resp.Diagnostics.Append(req.State.Get(ctx, &plan)...)
 
-	diags := req.State.Get(ctx, &plan)
-	resp.Diagnostics.Append(diags...)
-
-	if resp.Diagnostics.HasError() {
-		return
-	}
-
-	timeout, diags := at.client.timeouts.Read(ctx, DefaultTimeout)
-	resp.Diagnostics.Append(diags...)
-
-	if resp.Diagnostics.HasError() {
-		return
-	}
-
-	var r *getAgentTokenResponse
-	err := retry.RetryContext(ctx, timeout, func() *retry.RetryError {
-		var err error
-		r, err = getAgentToken(ctx,
-			at.client.genqlient,
-			fmt.Sprintf("%s/%s", at.client.organization, plan.Uuid.ValueString()),
-		)
-
-		if err != nil {
-			if isRetryableError(err) {
-				return retry.RetryableError(err)
-			}
-			return retry.NonRetryableError(err)
-		}
-
-		return nil
-	})
+	agentToken, err := getAgentToken(ctx, at.client.genqlient, fmt.Sprintf("%s/%s", at.client.organization, plan.Uuid.ValueString()))
 
 	if err != nil {
-		resp.Diagnostics.AddError(
-			"Unable to read agent token",
-			fmt.Sprintf("Unable to read agent token: %s", err.Error()),
-		)
+		resp.Diagnostics.AddError(err.Error(), err.Error())
 	}
-
-	if r == nil {
+	if agentToken == nil {
 		resp.Diagnostics.AddError("Agent token not found", "Removing from state")
 		resp.State.RemoveResource(ctx)
 		return
 	}
 
-	state.Description = types.StringPointerValue(r.AgentToken.Description)
-	state.Id = types.StringValue(r.AgentToken.Id)
+	state.Description = types.StringPointerValue(agentToken.AgentToken.Description)
+	state.Id = types.StringValue(agentToken.AgentToken.Id)
 	state.Token = plan.Token // token is never returned after creation so use the existing value in state
-	state.Uuid = types.StringValue(r.AgentToken.Uuid)
+	state.Uuid = types.StringValue(agentToken.AgentToken.Uuid)
 
 	resp.Diagnostics.Append(resp.State.Set(ctx, &state)...)
 }

diff --git a/buildkite/resource_agent_token_test.go b/buildkite/resource_agent_token_test.go
@@ -6,109 +6,80 @@ import (
 	"strings"
 	"testing"
 
-	"github.com/hashicorp/terraform-plugin-testing/helper/acctest"
 	"github.com/hashicorp/terraform-plugin-testing/helper/resource"
 	"github.com/hashicorp/terraform-plugin-testing/terraform"
 )
 
-func TestAccBuildkiteAgentToken(t *testing.T) {
-	basic := func(name string) string {
-		return fmt.Sprintf(`
-		provider "buildkite" {
-			timeouts {
-				create = "10s"
-				read = "10s"
-				update = "10s"
-				delete = "10s"
-			}
-		}
-
-		resource "buildkite_agent_token" "foobar" {
-			description = "Acceptance Test %s"
-		}
-		`, name)
-	}
-
-	// Confirm that we can create a new agent token, and then delete it without error
-	t.Run("adds an agent token", func(t *testing.T) {
-		var resourceToken AgentTokenNode
-		randName := acctest.RandString(10)
-
-		check := resource.ComposeAggregateTestCheckFunc(
-			// Confirm the token exists in the buildkite API
-			testAccCheckAgentTokenExists("buildkite_agent_token.foobar", &resourceToken),
-			// Confirm the token has the correct values in Buildkite's system
-			testAccCheckAgentTokenRemoteValues(&resourceToken, fmt.Sprintf("Acceptance Test %s", randName)),
-			// Confirm the token has the correct values in terraform state
-			resource.TestCheckResourceAttr("buildkite_agent_token.foobar", "description", fmt.Sprintf("Acceptance Test %s", randName)),
-			resource.TestCheckResourceAttrSet("buildkite_agent_token.foobar", "id"),
-			resource.TestCheckResourceAttrSet("buildkite_agent_token.foobar", "token"),
-			resource.TestCheckResourceAttrSet("buildkite_agent_token.foobar", "uuid"),
-		)
-
-		checkRefresh := resource.ComposeAggregateTestCheckFunc(
-			// Confirm the token has the correct values in terraform state
-			resource.TestCheckResourceAttrSet("buildkite_agent_token.foobar", "id"),
-			resource.TestCheckResourceAttrSet("buildkite_agent_token.foobar", "token"),
-			resource.TestCheckResourceAttrSet("buildkite_agent_token.foobar", "uuid"),
-		)
-
-		resource.ParallelTest(t, resource.TestCase{
-			PreCheck:                 func() { testAccPreCheck(t) },
-			ProtoV6ProviderFactories: protoV6ProviderFactories(),
-			CheckDestroy:             testAccCheckAgentTokenResourceDestroy,
-			Steps: []resource.TestStep{
-				{
-					Config: basic(randName),
-					Check:  check,
-				},
-				{
-					RefreshState: true,
-					PlanOnly:     true,
-					Check:        checkRefresh,
-				},
+// Confirm that we can create a new agent token, and then delete it without error
+func TestAccAgentToken_add_remove(t *testing.T) {
+	t.Parallel()
+	var resourceToken AgentTokenNode
+
+	resource.Test(t, resource.TestCase{
+		PreCheck:                 func() { testAccPreCheck(t) },
+		ProtoV6ProviderFactories: protoV6ProviderFactories(),
+		CheckDestroy:             testAccCheckAgentTokenResourceDestroy,
+		Steps: []resource.TestStep{
+			{
+				Config: testAccAgentTokenConfigBasic("foo"),
+				Check: resource.ComposeAggregateTestCheckFunc(
+					// Confirm the token exists in the buildkite API
+					testAccCheckAgentTokenExists("buildkite_agent_token.foobar", &resourceToken),
+					// Confirm the token has the correct values in Buildkite's system
+					testAccCheckAgentTokenRemoteValues(&resourceToken, "Acceptance Test foo"),
+					// Confirm the token has the correct values in terraform state
+					resource.TestCheckResourceAttr("buildkite_agent_token.foobar", "description", "Acceptance Test foo"),
+					resource.TestCheckResourceAttrSet("buildkite_agent_token.foobar", "id"),
+					resource.TestCheckResourceAttrSet("buildkite_agent_token.foobar", "token"),
+					resource.TestCheckResourceAttrSet("buildkite_agent_token.foobar", "uuid"),
+				),
 			},
-		})
+			{
+				RefreshState: true,
+				PlanOnly:     true,
+				Check: resource.ComposeAggregateTestCheckFunc(
+					// Confirm the token has the correct values in terraform state
+					resource.TestCheckResourceAttrSet("buildkite_agent_token.foobar", "id"),
+					resource.TestCheckResourceAttrSet("buildkite_agent_token.foobar", "token"),
+					resource.TestCheckResourceAttrSet("buildkite_agent_token.foobar", "uuid"),
+				),
+			},
+		},
 	})
+}
 
-	// Confirm that we can create a new agent token, and then update the description
-	// Technically tokens can't be updated, so this will actuall do a delete+create
-	t.Run("updates an agent token", func(t *testing.T) {
-		var resourceToken AgentTokenNode
-		randName := acctest.RandString(10)
-		randNameUpdated := acctest.RandString(10)
-
-		check := resource.ComposeAggregateTestCheckFunc(
-			// Confirm the token exists in the buildkite API
-			testAccCheckAgentTokenExists("buildkite_agent_token.foobar", &resourceToken),
-			// Quick check to confirm the local state is correct before we update it
-			resource.TestCheckResourceAttr("buildkite_agent_token.foobar", "description", fmt.Sprintf("Acceptance Test %s", randName)),
-		)
-
-		checkUpdated := resource.ComposeAggregateTestCheckFunc(
-			// Confirm the token exists in the buildkite API
-			testAccCheckAgentTokenExists("buildkite_agent_token.foobar", &resourceToken),
-			// Confirm the token has the updated values in Buildkite's system
-			testAccCheckAgentTokenRemoteValues(&resourceToken, fmt.Sprintf("Acceptance Test %s", randNameUpdated)),
-			// Confirm the token has the updated values in terraform state
-			resource.TestCheckResourceAttr("buildkite_agent_token.foobar", "description", fmt.Sprintf("Acceptance Test %s", randNameUpdated)),
-		)
-
-		resource.ParallelTest(t, resource.TestCase{
-			PreCheck:                 func() { testAccPreCheck(t) },
-			ProtoV6ProviderFactories: protoV6ProviderFactories(),
-			CheckDestroy:             testAccCheckAgentTokenResourceDestroy,
-			Steps: []resource.TestStep{
-				{
-					Config: basic(randName),
-					Check:  check,
-				},
-				{
-					Config: basic(randNameUpdated),
-					Check:  checkUpdated,
-				},
+// Confirm that we can create a new agent token, and then update the description
+// Technically tokens can't be updated, so this will actuall do a delete+create
+func TestAccAgentToken_update(t *testing.T) {
+	t.Parallel()
+	var resourceToken AgentTokenNode
+
+	resource.Test(t, resource.TestCase{
+		PreCheck:                 func() { testAccPreCheck(t) },
+		ProtoV6ProviderFactories: protoV6ProviderFactories(),
+		CheckDestroy:             testAccCheckAgentTokenResourceDestroy,
+		Steps: []resource.TestStep{
+			{
+				Config: testAccAgentTokenConfigBasic("foo"),
+				Check: resource.ComposeAggregateTestCheckFunc(
+					// Confirm the token exists in the buildkite API
+					testAccCheckAgentTokenExists("buildkite_agent_token.foobar", &resourceToken),
+					// Quick check to confirm the local state is correct before we update it
+					resource.TestCheckResourceAttr("buildkite_agent_token.foobar", "description", "Acceptance Test foo"),
+				),
 			},
-		})
+			{
+				Config: testAccAgentTokenConfigBasic("bar"),
+				Check: resource.ComposeAggregateTestCheckFunc(
+					// Confirm the token exists in the buildkite API
+					testAccCheckAgentTokenExists("buildkite_agent_token.foobar", &resourceToken),
+					// Confirm the token has the updated values in Buildkite's system
+					testAccCheckAgentTokenRemoteValues(&resourceToken, "Acceptance Test bar"),
+					// Confirm the token has the updated values in terraform state
+					resource.TestCheckResourceAttr("buildkite_agent_token.foobar", "description", "Acceptance Test bar"),
+				),
+			},
+		},
 	})
 }
 
@@ -165,6 +136,15 @@ func testAccCheckAgentTokenRemoteValues(resourceToken *AgentTokenNode, descripti
 	}
 }
 
+func testAccAgentTokenConfigBasic(description string) string {
+	config := `
+		resource "buildkite_agent_token" "foobar" {
+			description = "Acceptance Test %s"
+		}
+	`
+	return fmt.Sprintf(config, description)
+}
+
 // verifies the agent token has been destroyed
 func testAccCheckAgentTokenResourceDestroy(s *terraform.State) error {
 	for _, rs := range s.RootModule().Resources {