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

refactoring suggestion - migration table name is hardcoded in both pkg/spanner and cmd/ #50

Open
fcostin opened this issue Apr 25, 2022 · 2 comments

Comments

@fcostin
Copy link

fcostin commented Apr 25, 2022

while reviewing the code I noticed a minor issue with internal code structure, does not impact functionality of wrench tool:

Currently, it is not clear which layer of the code is responsible for choosing the default migration table name:

  1. the public API exposed by pkg/spanner Client has a tableName parameter on most operations that interact with the migration table, with the exception of TruncateAllTables, which hardcodes the table name instead of letting the caller control it:
    if t.TableName == "SchemaMigrations" {
  2. cmd/migrate.go also hardcodes the migration table name:
    migrationTableName = "SchemaMigrations"

The code structure could be slightly cleaner if only one layer of the code was responsible for choosing the migration table name.

I can think of a few options for refactoring to clarify this, but some have potential downsides of changing the API of exported methods of pkg/spanner Client . If it is important to not make breaking changes to pkg/spanner Client API (if users are using that as a library from their projects without using wrench command line tool) then it might be best not to do that.

Suggested options:

option description code structure breaking change to wrench tool? breaking change to exported wrench pkg/spanner Client API?
a current structure slightly unclear NA, no change NA, no change
b add tableName to TruncateAllTables slightly clearer no change breaking change!
c add TruncateAllTablesV2 with tableName, deprecate TruncateAllTables slightly clearer no change no change
d remove tableName from pkg/spanner Client, allow custom migration table name to be set using pkg/spanner Config slightly clearer no change breaking change!
@fcostin
Copy link
Author

fcostin commented Apr 25, 2022

I have raised a draft PR to explore what "option d" suggested above could look like: #51

that proposal has downside of being a breaking change to pkg/spanner Client API.

@asetty
Copy link

asetty commented Aug 22, 2023

Kind of on topic here, I have a use case where I'd like to manage migration separately for multiple sets of tables colocated in the same DB. We could add a migration table flag to the CLI tool to support this. I started making changes on a branch in my fork, but will need to add some testing probably.

Also some error checking for maybe:

  1. migrations table is valid ( in case of typos)
  2. all tables modified in migration do not conflict with other migration table's tables

One idea I have is creating a owners table that maps migration table to DB table in order to check this.
I may create a separate issue, but would be interested if someone replied here. It would be nice to merge your PR #72 first

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

No branches or pull requests

2 participants