cockroachdb / terraform-provider-cockroach

Terraform provider for CockroachDB Cloud
Apache License 2.0
57 stars 12 forks source link

[CC-9265]Add database resource #82

Closed jenngeorge closed 1 year ago

jenngeorge commented 1 year ago

Add a database resource, which manages a SQL database for a cluster.

jenngeorge commented 1 year ago
                Required: true,
            },
            "id": schema.StringAttribute{

Can you add a description to this field that says what the ID format is? Users will need to know for import.

Added! Also added descriptions to the computed ID fields for the allow list and sql user resources.

    }

    // Since the state may have come from an import, we need to retrieve

Imports aren't the only reason we need to read the actual state. We need to see if anything changed outside of Terraform, like if the database or underlying cluster have been deleted. I see you account for that below, which is great.

Updated the comment!

_internal/provider/database_resource.go line 294 at r1 (raw file):_

}

func loadDatabaseToTerraformState(databaseObj *client.ApiDatabase, state, plan *Database) {

Do you just need plan here for the cluster ID since it isn't contained in the database model? How does that work in the Update case? Where is the plan's ID getting set, and are you sure it's going to reflect the updated name? Because you could get ID collision if the ID isn't updated and the user creates a new database with the old name. I wonder if it'd be better to just pass in the cluster ID instead of the plan and calculate the database ID here.

Good idea, updated to pass in the cluster ID. I also tested the following scenarios in staging (took notes here):

jenngeorge commented 1 year ago

Thanks and TFTR!