From a254414cc75f4168262b6643013550c924c1402e Mon Sep 17 00:00:00 2001 From: Jarryd Tilbrook Date: Wed, 30 Aug 2023 11:41:33 +0800 Subject: [PATCH] Revert "SUP-1402: Agent token resource retries (#382)" This reverts commit deb70c333939f24c23ab577a911d71f1026e2ca0. --- CHANGELOG.md | 3 +- buildkite/resource_agent_token.go | 139 ++++---------------- buildkite/resource_agent_token_test.go | 170 +++++++++++-------------- 3 files changed, 98 insertions(+), 214 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index df5676ad..1273e054 100644 --- 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 index 9f97552c..ab70edcd 100644 --- 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 index 27ffbfd4..b2ab6545 100644 --- 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 {