Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow schema rename #450

Merged
merged 2 commits into from
Feb 21, 2024
Merged

Allow schema rename #450

merged 2 commits into from
Feb 21, 2024

Conversation

bobbyiliev
Copy link
Contributor

@bobbyiliev bobbyiliev commented Jan 30, 2024

Implementing the schema rename by itself is straight forward, the challenging part is all of the handling of all of the dependant resources, eg all resources that use the SchemaNameSchema.

Closes #334

@@ -13,7 +13,7 @@ import (
)

var schemaSchema = map[string]*schema.Schema{
"name": ObjectNameSchema("schema", true, true),
"name": ObjectNameSchema("schema", true, false),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem here is that if we change the schema name to not be ForceNew, then if someone actually wants to change the name of the schema they will not be able to do that. So there needs to be a mechanism where schema renames are allowed but also they don't break all of the objects that reference that schema.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. How does this work in other database providers, like PostgreSQL and Snowflake?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Snowflake does force recreate the dependant objects, eg:

https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/d301b200b45398a1bf1e6e4127983912ba1a52a8/pkg/resources/table.go#L27-L32

They also have some other bugs that affect the way they handle schemas:

Snowflake-Labs/terraform-provider-snowflake#845

The Postgres providers that I've checked do not seem to support this at all, eg. they don't have resources that depend on schema names.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One option would be to leave the ForceNew as true and then instruct users that they can add an ignore block to the child objects, eg:

resource "materialize_database" "db" {
  name = "db"
}
resource "materialize_schema" "schema" {
  name = "test_schema3"
  database_name = materialize_database.db.name
}

resource "materialize_table" "test_table" {
  name = "test_table"
  schema_name = materialize_schema.schema.name
  # Ignore schema name rename
  lifecycle {
    ignore_changes = [schema_name]
  }
  database_name = materialize_database.db.name
  column {
    name = "id"
    type = "int"
  }
  column {
    name = "name"
    type = "text"
  }
}

That way the schema rename will work as expected, and the child resources will not be recreated and after the rename, the ignore block can be removed as the schema name will be updated correctly in the state file thanks to the read func.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm way out of my depth here, but it seems like the easy answer is to not do anything special, which means users have to add the lifecycle ignore blocks like you suggested. If we do decide to set ForceNew to false, what other mechanism would we provide for schema renames?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be question we could ask the cloud team, maybe they have suggestions since they have a lot of collective IaC experience?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After the discussion, I would say that we can take the following plan of action:

  • For the short term: Merge this PR as it is as it will still allow people to rename schemas without having to drop all of their dependent objects some manual work by using the lifecycle { ignore_changes = [schema_name] } syntax
  • For the long term: Implement schema_id which can be used along side schema_name, I've opened a tracking issue for this as it will be a pretty significant change as 80% of all resources actually depend on the schema_name at the moment: [Epic] Introduce schema_id for all object that use schema_name #472

Open to suggestions! Tagging @jubrad as schema_id was his idea 👏

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@benesch if you are ok with the above ^ we could go ahead and merge this, and after that plan the larger refactor in #472

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yes, sorry, absolutely okay with this! It's great to have an MVP!

@bobbyiliev bobbyiliev marked this pull request as ready for review February 14, 2024 16:01
@bobbyiliev bobbyiliev requested a review from a team as a code owner February 14, 2024 16:01
@bobbyiliev bobbyiliev requested review from RobinClowers and removed request for a team February 14, 2024 16:01
@bobbyiliev bobbyiliev changed the title [WIP] Allow schema rename Allow schema rename Feb 15, 2024
@bobbyiliev bobbyiliev requested a review from benesch February 21, 2024 07:36
@bobbyiliev bobbyiliev merged commit fd37171 into main Feb 21, 2024
5 checks passed
@bobbyiliev bobbyiliev deleted the Allow-Schema-Rename branch February 21, 2024 07:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow Schema Rename
3 participants