Skip to content

Commit

Permalink
fix: handle 404s during Read() correctly
Browse files Browse the repository at this point in the history
re: #53

When a resource in the statefile has been deleted out of band, such as someone on the console or
using pscale cli deleting it, terraform should remove it from state and plan to recreate it.
However, we were treating 404s as fatal.

To solve this, the `Read()` method in each of our four resources now checks for 404 and removes the
resource from state so that terraform will offer to recreate the resource

This PR also adds initial acceptance test coverage for `database`, `password`, and `branch`
resources, including tests for the out-of-band deletion handling fixed by this PR. The `backup`
resource is currently not working and so tests for it will be added in later PRs.
  • Loading branch information
joemiller committed Aug 21, 2024
1 parent a5e4875 commit 9486a5c
Show file tree
Hide file tree
Showing 15 changed files with 413 additions and 34 deletions.
4 changes: 3 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -34,4 +34,6 @@ website/vendor
# Keep windows files with windows line endings
*.winfile eol=crlf

.env
.env
.envrc
terraform-provider-planetscale
2 changes: 1 addition & 1 deletion docs/data-sources/password.md
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ output "password" {
### Required

- `branch` (String) The branch this password belongs to..
- `database` (String) The datanase this branch password belongs to.
- `database` (String) The database this branch password belongs to.
- `id` (String) The ID for the password.
- `organization` (String) The organization this database branch password belongs to.

Expand Down
2 changes: 1 addition & 1 deletion docs/data-sources/passwords.md
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ Read-Only:
- `actor` (Attributes) The actor that created this branch. (see [below for nested schema](#nestedatt--passwords--actor))
- `branch` (String) The branch this password belongs to..
- `created_at` (String) When the password was created.
- `database` (String) The datanase this branch password belongs to.
- `database` (String) The database this branch password belongs to.
- `database_branch` (Attributes) The branch this password is allowed to access. (see [below for nested schema](#nestedatt--passwords--database_branch))
- `deleted_at` (String) When the password was deleted.
- `expires_at` (String) When the password will expire.
Expand Down
2 changes: 1 addition & 1 deletion docs/resources/password.md
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ output "password" {
### Required

- `branch` (String) The branch this password belongs to.
- `database` (String) The datanase this branch password belongs to.
- `database` (String) The database this branch password belongs to.
- `organization` (String) The organization this database branch password belongs to.

### Optional
Expand Down
2 changes: 2 additions & 0 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -322,6 +322,8 @@ golang.org/x/term v0.0.0-20210927222741-03fcf44c2211/go.mod h1:jbD1KX2456YbFQfuX
golang.org/x/term v0.2.0/go.mod h1:TVmDHMZPmdnySmBfhjOoOdhjzdE1h4u1VwSiw2l1Nuc=
golang.org/x/term v0.5.0/go.mod h1:jMB1sMXY+tzblOD4FWmEbocvup2/aLOaQEp7JmGp78k=
golang.org/x/term v0.6.0/go.mod h1:m6U89DPEgQRMq3DNkDClhWw02AUbt2daBVO4cn4Hv9U=
golang.org/x/term v0.13.0 h1:bb+I9cTfFazGW51MZqBVmZy7+JEJMouUHTUSKVQLBek=
golang.org/x/term v0.13.0/go.mod h1:LTmsnFJwVN6bCy1rVCoS+qHT1HhALEFxKncY3WNNh4U=
golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ=
golang.org/x/text v0.3.2/go.mod h1:bEr9sfX3Q8Zfm5fL9x+3itogRgK3+ptLWKqgva+5dAk=
golang.org/x/text v0.3.3/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ=
Expand Down
23 changes: 17 additions & 6 deletions internal/provider/backup_resource.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,10 @@ import (
)

// Ensure provider defined types fully satisfy framework interfaces.
var _ resource.Resource = &backupResource{}
var _ resource.ResourceWithImportState = &backupResource{}
var (
_ resource.Resource = &backupResource{}
_ resource.ResourceWithImportState = &backupResource{}
)

func newBackupResource() resource.Resource {
return &backupResource{}
Expand Down Expand Up @@ -101,25 +103,29 @@ Known limitations:
Required: true,
PlanModifiers: []planmodifier.String{
stringplanmodifier.RequiresReplace(),
}},
},
},
"database": schema.StringAttribute{
Description: "The database to which the branch being backed up belongs to.",
Required: true,
PlanModifiers: []planmodifier.String{
stringplanmodifier.RequiresReplace(),
}},
},
},
"branch": schema.StringAttribute{
Description: "The branch being backed up.",
Required: true,
PlanModifiers: []planmodifier.String{
stringplanmodifier.RequiresReplace(),
}},
},
},
"name": schema.StringAttribute{
Description: "The name of the backup.",
Required: true,
PlanModifiers: []planmodifier.String{
stringplanmodifier.RequiresReplace(),
}},
},
},
"backup_policy": schema.SingleNestedAttribute{
Description: "The policy used by the backup.",
Required: true,
Expand Down Expand Up @@ -281,6 +287,11 @@ func (r *backupResource) Read(ctx context.Context, req resource.ReadRequest, res

res, err := r.client.GetBackup(ctx, org.ValueString(), database.ValueString(), branch.ValueString(), id.ValueString())
if err != nil {
if notFoundErr, ok := err.(*planetscale.GetBackupRes404); ok {
tflog.Warn(ctx, fmt.Sprintf("Backup not found, removing from state: %s", notFoundErr.Message))
resp.State.RemoveResource(ctx)
return
}
resp.Diagnostics.AddError("Client Error", fmt.Sprintf("Unable to read backup, got error: %s", err))
return
}
Expand Down
64 changes: 64 additions & 0 deletions internal/provider/branch_resource_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
package provider

import (
"fmt"
"testing"

"github.com/hashicorp/terraform-plugin-testing/helper/acctest"
"github.com/hashicorp/terraform-plugin-testing/helper/resource"
)

func TestAccBranchResource(t *testing.T) {
// TODO: This test currently fails because the provider returns immediately
// after DB creation but the DB is still pending and so the branch creation
// will fail. Unblock and finish this test once this issue is resolved.
t.Skip()

dbName := acctest.RandomWithPrefix("testacc-branch")
branchName := "two"

resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
ProtoV6ProviderFactories: testAccProtoV6ProviderFactories,
Steps: []resource.TestStep{
// Create and Read testing
{
Config: testAccBranchResourceConfig(dbName, branchName),
Check: resource.ComposeAggregateTestCheckFunc(
resource.TestCheckResourceAttr("planetscale_branch.two", "name", branchName),
resource.TestCheckResourceAttr("planetscale_branch.two", "parent_branch", "main"),
resource.TestCheckResourceAttr("planetscale_branch.two", "sharded", "false"),
),
},
// ImportState testing
{
ResourceName: "planetscale_branch.test",
ImportState: true,
ImportStateVerify: true,
},
// Update and Read testing
// TODO: Implement an update test.
// Delete testing automatically occurs in TestCase
},
})
}

// TODO: implement an out of bound deletion test like we have in the password and database tests.

func testAccBranchResourceConfig(dbName, branchName string) string {
return fmt.Sprintf(`
resource "planetscale_database" "test" {
organization = "%s"
name = "%s"
cluster_size = "PS-10"
default_branch = "main"
}
resource "planetscale_branch" "two" {
organization = "%s"
database = planetscale_database.test.name
name = "%s"
parent_branch = planetscale_database.test.default_branch
}
`, testAccOrg, dbName, testAccOrg, branchName)
}
17 changes: 12 additions & 5 deletions internal/provider/database_resource.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,10 @@ import (
)

// Ensure provider defined types fully satisfy framework interfaces.
var _ resource.Resource = &databaseResource{}
var _ resource.ResourceWithImportState = &databaseResource{}
var (
_ resource.Resource = &databaseResource{}
_ resource.ResourceWithImportState = &databaseResource{}
)

func newDatabaseResource() resource.Resource {
return &databaseResource{}
Expand Down Expand Up @@ -404,6 +406,11 @@ func (r *databaseResource) Read(ctx context.Context, req resource.ReadRequest, r

res, err := r.client.GetDatabase(ctx, org.ValueString(), name.ValueString())
if err != nil {
if notFoundErr, ok := err.(*planetscale.GetDatabaseRes404); ok {
tflog.Warn(ctx, fmt.Sprintf("Database not found, removing from state: %s", notFoundErr.Message))
resp.State.RemoveResource(ctx)
return
}
resp.Diagnostics.AddError("Client Error", fmt.Sprintf("Unable to read database, got error: %s", err))
return
}
Expand Down Expand Up @@ -489,12 +496,13 @@ func (r *databaseResource) Delete(ctx context.Context, req resource.DeleteReques
return
}

res, err := r.client.DeleteDatabase(ctx, org.ValueString(), name.ValueString())
_, err := r.client.DeleteDatabase(ctx, org.ValueString(), name.ValueString())
if err != nil {
resp.Diagnostics.AddError("Client Error", fmt.Sprintf("Unable to delete database, got error: %s", err))
return
}
_ = res
// Mark dependent resources for recreation
resp.State.RemoveResource(ctx)
}

func (r *databaseResource) ImportState(ctx context.Context, req resource.ImportStateRequest, resp *resource.ImportStateResponse) {
Expand All @@ -510,5 +518,4 @@ func (r *databaseResource) ImportState(ctx context.Context, req resource.ImportS

resp.Diagnostics.Append(resp.State.SetAttribute(ctx, path.Root("organization"), idParts[0])...)
resp.Diagnostics.Append(resp.State.SetAttribute(ctx, path.Root("name"), idParts[1])...)

}
98 changes: 98 additions & 0 deletions internal/provider/database_resource_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
package provider

import (
"context"
"fmt"
"testing"

"github.com/hashicorp/terraform-plugin-testing/helper/acctest"
"github.com/hashicorp/terraform-plugin-testing/helper/resource"
)

func TestAccDatabaseResource(t *testing.T) {
dbName := acctest.RandomWithPrefix("testacc-db")

resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
ProtoV6ProviderFactories: testAccProtoV6ProviderFactories,
Steps: []resource.TestStep{
// Create and Read testing
{
Config: testAccDatabaseResourceConfig(dbName, "PS-10"),
Check: resource.ComposeAggregateTestCheckFunc(
// resource.TestCheckResourceAttr("planetscale_database.test", "id", "todo"),
resource.TestCheckResourceAttr("planetscale_database.test", "production_branches_count", "1"),
resource.TestCheckResourceAttr("planetscale_database.test", "default_branch", "main"),
resource.TestCheckResourceAttr("planetscale_database.test", "cluster_size", "PS-10"),
),
},
// ImportState testing
{
ResourceName: "planetscale_database.test",
ImportStateId: fmt.Sprintf("%s,%s", testAccOrg, dbName),
ImportState: true,
ImportStateVerify: true,
// TODO: API does not return cluster_size which causes a diff on import. When fixed, remove this:
ImportStateVerifyIgnore: []string{"cluster_size"},
},
// Update and Read testing
{
Config: testAccDatabaseResourceConfig(dbName, "PS-20"),
Check: resource.ComposeAggregateTestCheckFunc(
// resource.TestCheckResourceAttr("planetscale_database.test", "id", "todo"),
resource.TestCheckResourceAttr("planetscale_database.test", "production_branches_count", "1"),
resource.TestCheckResourceAttr("planetscale_database.test", "default_branch", "main"),
resource.TestCheckResourceAttr("planetscale_database.test", "cluster_size", "PS-20"),
),
},
// Delete testing automatically occurs in TestCase
},
})
}

// TestAccDatabaseResource_outOfBandDelete tests the out-of-band deletion of the database.
// In this test we simulate the remote database has been deleted out of band, perhaps by
// a user on the console or using pscale CLI.
// https://github.com/planetscale/terraform-provider-planetscale/issues/53
func TestAccDatabaseResource_OutOfBandDelete(t *testing.T) {
dbName := acctest.RandomWithPrefix("testacc-db-oob")

resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
ProtoV6ProviderFactories: testAccProtoV6ProviderFactories,
Steps: []resource.TestStep{
// Create and Read testing
{
Config: testAccDatabaseResourceConfig(dbName, "PS-10"),
Check: resource.ComposeAggregateTestCheckFunc(
resource.TestCheckResourceAttr("planetscale_database.test", "production_branches_count", "1"),
resource.TestCheckResourceAttr("planetscale_database.test", "default_branch", "main"),
resource.TestCheckResourceAttr("planetscale_database.test", "cluster_size", "PS-10"),
),
},
// Test out-of-bands deletion of the database should produce a plan to recreate, not error.
{
ResourceName: "planetscale_database.test",
RefreshState: true,
ExpectNonEmptyPlan: true,
PreConfig: func() {
ctx := context.Background()
_, err := testAccAPIClient.DeleteDatabase(ctx, testAccOrg, dbName)
if err != nil {
t.Fatalf("PreConfig: failed to delete database: %s", err)
}
},
},
},
})
}

func testAccDatabaseResourceConfig(dbName string, clusterSize string) string {
return fmt.Sprintf(`
resource "planetscale_database" "test" {
organization = "%s"
name = "%s"
cluster_size = "%s"
}
`, testAccOrg, dbName, clusterSize)
}
2 changes: 1 addition & 1 deletion internal/provider/models_data_source.go
Original file line number Diff line number Diff line change
Expand Up @@ -1191,7 +1191,7 @@ func passwordDataSourceSchemaAttribute(computedName bool) map[string]schema.Attr
Required: !computedName, Computed: computedName,
},
"database": schema.StringAttribute{
Description: "The datanase this branch password belongs to.",
Description: "The database this branch password belongs to.",
Required: !computedName, Computed: computedName,
},
"branch": schema.StringAttribute{
Expand Down
3 changes: 1 addition & 2 deletions internal/provider/organization_regions_data_source_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,13 @@ var regions = []string{
}

func TestAccOrganizationRegionsDataSource(t *testing.T) {
orgName := "planetscale-terraform-testing"
resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
ProtoV6ProviderFactories: testAccProtoV6ProviderFactories,
Steps: []resource.TestStep{
// Read testing
{
Config: testAccOrganizationRegionsDataSourceConfig(orgName),
Config: testAccOrganizationRegionsDataSourceConfig(testAccOrg),
Check: resource.ComposeAggregateTestCheckFunc(
resource.TestCheckResourceAttrWith("data.planetscale_organization_regions.test", "regions.#", checkIntegerMin(1)),
resource.TestCheckResourceAttrWith("data.planetscale_organization_regions.test", "regions.0.slug", checkOneOf(regions...)),
Expand Down
2 changes: 1 addition & 1 deletion internal/provider/organizations_data_source_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ func TestAccOrganizationsDataSource(t *testing.T) {
Config: testAccOrganizationsDataSourceConfig,
Check: resource.ComposeAggregateTestCheckFunc(
resource.TestCheckResourceAttr("data.planetscale_organizations.test", "organizations.#", "1"),
resource.TestCheckResourceAttr("data.planetscale_organizations.test", "organizations.0.name", "planetscale-terraform-testing"),
resource.TestCheckResourceAttr("data.planetscale_organizations.test", "organizations.0.name", testAccOrg),
resource.TestCheckResourceAttrSet("data.planetscale_organizations.test", "organizations.0.admin_only_production_access"),
resource.TestCheckResourceAttrSet("data.planetscale_organizations.test", "organizations.0.billing_email"),
resource.TestCheckResourceAttrSet("data.planetscale_organizations.test", "organizations.0.can_create_databases"),
Expand Down
Loading

0 comments on commit 9486a5c

Please sign in to comment.