From 361b87ee33be3aa02e7d873030909e86a6006a9b Mon Sep 17 00:00:00 2001 From: Mark McDonnell Date: Wed, 25 Oct 2023 19:46:50 +0100 Subject: [PATCH] feat(resources/servicevcl): support importing a specific service version (#69) * refactor: separate each test case into separate test functions * ci: use latest golangci-lint * test(resources/servicevcl): validate import of specific service version * fix(resources/servicevcl): correctly handle import scenarios * refactor(resources/servicevcl): reduce number of statements in read method --- .github/workflows/test.yml | 2 +- .../resources/servicevcl/process_read.go | 61 ++++-- .../provider/resources/servicevcl/resource.go | 22 ++- .../tests/resources/service_vcl_test.go | 180 +++++++++++++----- 4 files changed, 186 insertions(+), 79 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 82d9102..c5f1207 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -29,7 +29,7 @@ jobs: - name: Run linters uses: golangci/golangci-lint-action@v3 with: - version: v1.54.2 # switch to "latest" once new release (post 1.55.0) is published. + version: latest args: "--verbose" generate: name: Generate diff --git a/internal/provider/resources/servicevcl/process_read.go b/internal/provider/resources/servicevcl/process_read.go index 10a86ae..f46d0c4 100644 --- a/internal/provider/resources/servicevcl/process_read.go +++ b/internal/provider/resources/servicevcl/process_read.go @@ -6,6 +6,7 @@ import ( "fmt" "net/http" + "github.com/fastly/fastly-go/fastly" "github.com/hashicorp/terraform-plugin-framework/resource" "github.com/hashicorp/terraform-plugin-framework/types" "github.com/hashicorp/terraform-plugin-log/tflog" @@ -53,26 +54,7 @@ func (r *Resource) Read(ctx context.Context, req resource.ReadRequest, resp *res return } - // NOTE: When importing a service there is no prior 'serviceVersion' in the state. - // So we presume the user wants to import the last active service serviceVersion. - // Which we retrieve from the GetServiceDetail call. - var ( - foundActive bool - serviceVersion int64 - ) - versions := clientResp.GetVersions() - for _, version := range versions { - if version.GetActive() { - serviceVersion = int64(version.GetNumber()) - foundActive = true - break - } - } - - if !foundActive { - // Use latest version if the user imports a service with no active versions. - serviceVersion = int64(versions[0].GetNumber()) - } + serviceVersion := readServiceVersion(state, clientResp) api := helpers.API{ Client: r.client, @@ -114,6 +96,45 @@ func (r *Resource) Read(ctx context.Context, req resource.ReadRequest, resp *res tflog.Debug(ctx, "Read", map[string]any{"state": fmt.Sprintf("%#v", state)}) } +// readServiceVersion returns the service version. +// +// The returned value depends on if we're in an import scenario. +// +// When importing a service there might be no prior 'serviceVersion' in state. +// If the user imports using the `ID@VERSION` syntax, then there will be. +// This is because `ImportState()` in ./resource.go makes sure it's set. +// +// So we check if the attribute is null or not. +// +// If it's null, then we'll presume the user wants the last active version. +// Which we retrieve from the GetServiceDetail call. +// We fallback to the latest version if there is no prior active version. +// +// Otherwise we'll use whatever version they specified in their import. +func readServiceVersion(state *models.ServiceVCL, clientResp *fastly.ServiceDetail) int64 { + var serviceVersion int64 + + if state.Version.IsNull() { + var foundActive bool + versions := clientResp.GetVersions() + for _, version := range versions { + if version.GetActive() { + serviceVersion = int64(version.GetNumber()) + foundActive = true + break + } + } + if !foundActive { + // Use latest version if the user imports a service with no active versions. + serviceVersion = int64(versions[0].GetNumber()) + } + } else { + serviceVersion = state.Version.ValueInt64() + } + + return serviceVersion +} + func readSettings(ctx context.Context, state *models.ServiceVCL, resp *resource.ReadResponse, api helpers.API) error { serviceID := state.ID.ValueString() serviceVersion := int32(state.Version.ValueInt64()) diff --git a/internal/provider/resources/servicevcl/resource.go b/internal/provider/resources/servicevcl/resource.go index c94d26e..ae0a461 100644 --- a/internal/provider/resources/servicevcl/resource.go +++ b/internal/provider/resources/servicevcl/resource.go @@ -4,6 +4,8 @@ import ( "context" _ "embed" "fmt" + "strconv" + "strings" "github.com/fastly/fastly-go/fastly" "github.com/hashicorp/terraform-plugin-framework-validators/resourcevalidator" @@ -12,6 +14,7 @@ import ( "github.com/hashicorp/terraform-plugin-framework/resource/schema" "github.com/hashicorp/terraform-plugin-framework/resource/schema/booldefault" "github.com/hashicorp/terraform-plugin-framework/resource/schema/int64default" + "github.com/hashicorp/terraform-plugin-framework/types" "github.com/hashicorp/terraform-plugin-go/tftypes" "github.com/hashicorp/terraform-plugin-log/tflog" @@ -139,16 +142,15 @@ func (r *Resource) Configure(_ context.Context, req resource.ConfigureRequest, r // The service resource then iterates over all nested resources populating the // state for each nested resource. func (r *Resource) ImportState(ctx context.Context, req resource.ImportStateRequest, resp *resource.ImportStateResponse) { - // TODO: req.ID needs to be checked for format. - // Typically just a Service ID but can also be @ - // If the @ format is provided, then we need to parse the - // version and set it into the `version` attribute as well as `last_active`. - - // The ImportStatePassthroughID() call is a small helper function that simply - // checks for an empty ID value passed (and errors accordingly) and if there - // is no error it calls `resp.State.SetAttribute()` passing in the ADDRESS - // (which we hardcode to the `id` attribute) and the user provided ID value. - resource.ImportStatePassthroughID(ctx, path.Root("id"), req, resp) + id, version, found := strings.Cut(req.ID, "@") + if found { + v, err := strconv.ParseInt(version, 10, 64) + if err != nil { + return + } + resp.Diagnostics.Append(resp.State.SetAttribute(ctx, path.Root("version"), types.Int64Value(v))...) + } + resp.Diagnostics.Append(resp.State.SetAttribute(ctx, path.Root("id"), id)...) var state map[string]tftypes.Value err := resp.State.Raw.As(&state) diff --git a/internal/provider/tests/resources/service_vcl_test.go b/internal/provider/tests/resources/service_vcl_test.go index 20f468b..1adc529 100644 --- a/internal/provider/tests/resources/service_vcl_test.go +++ b/internal/provider/tests/resources/service_vcl_test.go @@ -14,6 +14,8 @@ import ( "github.com/integralist/terraform-provider-fastly-framework/internal/provider" ) +// The following test validates the standard service behaviours. +// e.g. creating/updating the resource and nested resources. func TestAccResourceServiceVCL(t *testing.T) { serviceName := fmt.Sprintf("tf-test-%s", acctest.RandString(10)) domain1Name := fmt.Sprintf("%s-tpff-1.integralist.co.uk", serviceName) @@ -21,23 +23,13 @@ func TestAccResourceServiceVCL(t *testing.T) { domain2Name := fmt.Sprintf("%s-tpff-2.integralist.co.uk", serviceName) domain2NameUpdated := fmt.Sprintf("%s-tpff-2-updated.integralist.co.uk", serviceName) - // Create a service with two domains. - // Also set `force_destroy = false`. - configCreate := fmt.Sprintf(` - resource "fastly_service_vcl" "test" { - name = "%s" - force_destroy = false - - domains = { - "example-1" = { - name = "%s" - }, - "example-2" = { - name = "%s" - }, - } - } - `, serviceName, domain1Name, domain2Name) + configCreate := configServiceVCLCreate(configServiceVCLCreateOpts{ + activate: true, + forceDestroy: false, + serviceName: serviceName, + domain1Name: domain1Name, + domain2Name: domain2Name, + }) // Update the first domain's comment + second domain's name (force_destroy = true). // We also change the order of the domains so the second is now first. @@ -155,9 +147,23 @@ func TestAccResourceServiceVCL(t *testing.T) { // Delete testing automatically occurs in TestCase }, }) +} + +// The following test validates the service deleted_at behaviour. +// i.e. if deleted_at is not empty, then remove the service resource. +func TestAccResourceServiceVCLDeletedAtCheck(t *testing.T) { + serviceName := fmt.Sprintf("tf-test-%s", acctest.RandString(10)) + domain1Name := fmt.Sprintf("%s-tpff-1.integralist.co.uk", serviceName) + domain2Name := fmt.Sprintf("%s-tpff-2.integralist.co.uk", serviceName) + + configCreate := configServiceVCLCreate(configServiceVCLCreateOpts{ + activate: true, + forceDestroy: true, + serviceName: serviceName, + domain1Name: domain1Name, + domain2Name: domain2Name, + }) - // NOTE: The following test validates the service deleted_at behaviour. - // i.e. if deleted_at is not empty, then remove the service resource. resource.Test(t, resource.TestCase{ PreCheck: func() { provider.TestAccPreCheck(t) }, ProtoV6ProviderFactories: provider.TestAccProtoV6ProviderFactories, @@ -172,37 +178,27 @@ func TestAccResourceServiceVCL(t *testing.T) { resource.TestCheckResourceAttr("fastly_service_vcl.test", "domains.%", "2"), resource.TestCheckResourceAttr("fastly_service_vcl.test", "domains.example-1.name", domain1Name), resource.TestCheckResourceAttr("fastly_service_vcl.test", "domains.example-2.name", domain2Name), - resource.TestCheckResourceAttr("fastly_service_vcl.test", "force_destroy", "false"), + resource.TestCheckResourceAttr("fastly_service_vcl.test", "force_destroy", "true"), resource.TestCheckResourceAttr("fastly_service_vcl.test", "stale_if_error", "false"), resource.TestCheckResourceAttr("fastly_service_vcl.test", "stale_if_error_ttl", "43200"), resource.TestCheckNoResourceAttr("fastly_service_vcl.test", "domains.example-1.comment"), resource.TestCheckNoResourceAttr("fastly_service_vcl.test", "domains.example-2.comment"), ), }, - // Update and Read testing - { - Config: configUpdate, - Check: resource.ComposeAggregateTestCheckFunc( - resource.TestCheckResourceAttr("fastly_service_vcl.test", "force_destroy", "true"), - resource.TestCheckResourceAttr("fastly_service_vcl.test", "domains.example-1.comment", domain1CommentAdded), - resource.TestCheckResourceAttr("fastly_service_vcl.test", "domains.example-2.name", domain2NameUpdated), - resource.TestCheckNoResourceAttr("fastly_service_vcl.test", "domains.example-2.comment"), - ), - }, // Trigger side-effect of deleting resource outside of Terraform. // We use the same config as previous TestStep (so no config changes). // // Because Terraform executes a refresh/plan after each test case, we // validate that the final plan is not empty using `ExpectNonEmptyPlan`. { - Config: configUpdate, + Config: configCreate, Check: resource.ComposeAggregateTestCheckFunc( func(s *terraform.State) error { if r, ok := s.RootModule().Resources["fastly_service_vcl.test"]; ok { if id, ok := r.Primary.Attributes["id"]; ok { apiClient := fastly.NewAPIClient(fastly.NewConfiguration()) ctx := fastly.NewAPIKeyContextFromEnv(helpers.APIKeyEnv) - version := int32(2) + version := int32(1) deactivateReq := apiClient.VersionAPI.DeactivateServiceVersion(ctx, id, version) _, httpResp, err := deactivateReq.Execute() if err != nil { @@ -225,29 +221,31 @@ func TestAccResourceServiceVCL(t *testing.T) { // Delete testing automatically occurs in TestCase }, }) +} + +// The following test validates the service type import behaviour. +// i.e. when importing a service, check the service type matches the resource. +// e.g. importing a Compute service ID into a VCL service resource. +func TestAccResourceServiceVCLImportServiceTypeCheck(t *testing.T) { + serviceName := fmt.Sprintf("tf-test-%s", acctest.RandString(10)) + domain1Name := fmt.Sprintf("%s-tpff-1.integralist.co.uk", serviceName) + domain2Name := fmt.Sprintf("%s-tpff-2.integralist.co.uk", serviceName) + + configCreate := configServiceVCLCreate(configServiceVCLCreateOpts{ + activate: false, + forceDestroy: true, + serviceName: serviceName, + domain1Name: domain1Name, + domain2Name: domain2Name, + }) - // NOTE: The following test validates the service type import behaviour. - // i.e. when importing a service, check the service type matches the resource. - // e.g. importing a Compute service ID into a VCL service resource. resource.Test(t, resource.TestCase{ PreCheck: func() { provider.TestAccPreCheck(t) }, ProtoV6ProviderFactories: provider.TestAccProtoV6ProviderFactories, Steps: []resource.TestStep{ // We need a resource to be created by Terraform so we can import into it. { - Config: fmt.Sprintf(` - resource "fastly_service_vcl" "test" { - name = "%s" - activate = false - force_destroy = true - - domains = { - "example" = { - name = "%s" - }, - } - } - `, serviceName, domain1Name), + Config: configCreate, }, // ImportState testing { @@ -269,3 +267,89 @@ func TestAccResourceServiceVCL(t *testing.T) { }, }) } + +// The following test validates importing a specific service version. +// e.g. terraform import fastly_service_vcl.test xxxxxxxxxxxxxxxxxxxx@2. +func TestAccResourceServiceVCLImportServiceVersion(t *testing.T) { + serviceName := fmt.Sprintf("tf-test-%s", acctest.RandString(10)) + domain1Name := fmt.Sprintf("%s-tpff-1.integralist.co.uk", serviceName) + domain2Name := fmt.Sprintf("%s-tpff-2.integralist.co.uk", serviceName) + + configCreate := configServiceVCLCreate(configServiceVCLCreateOpts{ + activate: true, + forceDestroy: true, + serviceName: serviceName, + domain1Name: domain1Name, + domain2Name: domain2Name, + }) + + resource.Test(t, resource.TestCase{ + PreCheck: func() { provider.TestAccPreCheck(t) }, + ProtoV6ProviderFactories: provider.TestAccProtoV6ProviderFactories, + Steps: []resource.TestStep{ + // We need a resource to be created by Terraform so we can import into it. + { + Config: configCreate, + }, + // Clone the service version and return the import ID to use. + { + ResourceName: "fastly_service_vcl.test", + ImportState: true, + ImportStateIdFunc: func(s *terraform.State) (string, error) { + if r, ok := s.RootModule().Resources["fastly_service_vcl.test"]; ok { + if id, ok := r.Primary.Attributes["id"]; ok { + apiClient := fastly.NewAPIClient(fastly.NewConfiguration()) + ctx := fastly.NewAPIKeyContextFromEnv(helpers.APIKeyEnv) + req := apiClient.VersionAPI.CloneServiceVersion(ctx, id, 1) + _, _, err := req.Execute() + if err != nil { + return "", fmt.Errorf("failed to clone service version: %w", err) + } + return id + "@2", nil + } + } + return "", nil + }, + ImportStateCheck: func(is []*terraform.InstanceState) error { + for _, s := range is { + serviceVersion := s.Attributes["version"] + if serviceVersion != "2" { + return fmt.Errorf("import failed: unexpected service version found: got %s, want 2", serviceVersion) + } + } + return nil + }, + }, + // Delete testing automatically occurs in TestCase + }, + }) +} + +type configServiceVCLCreateOpts struct { + activate, forceDestroy bool + serviceName, domain1Name, domain2Name string +} + +// configServiceVCLCreate returns a TF config that consists of a VCL service +// with two domains + configurable `activate` and `force_destroy` attributes. +// +// NOTE: We use this config for a lot of the tests. +// But occasionally we need to tweak the force_destroy attribute. +func configServiceVCLCreate(opts configServiceVCLCreateOpts) string { + return fmt.Sprintf(` + resource "fastly_service_vcl" "test" { + activate = %t + force_destroy = %t + name = "%s" + + domains = { + "example-1" = { + name = "%s" + }, + "example-2" = { + name = "%s" + }, + } + } + `, opts.activate, opts.forceDestroy, opts.serviceName, opts.domain1Name, opts.domain2Name) +}