WHAT

It would be great if we can add an install.sh script like in golangcli-lint

Exampe of Install.sh -> https://github.com/reviewdog/reviewdog/blob/master/install.sh

Example

jobs:
  lint:
    working_directory: ~/reviewdog
    docker:
      - image: circleci/golang:1.17
    steps:
      - checkout
      - run:
          name: System information
          command: 'echo "Golang $(go version)"'
      - run: 'curl -sfL https://raw.githubusercontent.com/reviewdog/reviewdog/master/install.sh| sh -s -- -b $(go env GOPATH)/bin'
      - run:
          command: |-
            set +o pipefail
            golint ./... | reviewdog -f=golint -name=golint-circleci -reporter=github-pr-review

WHY

It would make it easy to install mercari-wrench for Ci

Expected Behavior

Expected v1.2.0 and v1.2.1 release tags include binary Assets:

• wrench_darwin_amd64
• wrench_darwin_arm64
• wrench_linux_amd64
• wrench_linux_arm64

Allow developers to download such binary Assets directly.

Current Behavior

Not included Binary Assets of the recent release tags, v1.2.0 and v1.2.1. Only source files are available to download as release tag Assets.

https://github.com/cloudspannerecosystem/wrench/releases/tag/v1.2.0
https://github.com/cloudspannerecosystem/wrench/releases/tag/v1.2.1

Steps to Reproduce

1. RUN curl -L https://github.com/cloudspannerecosystem/wrench/releases/download/v1.2.0/wrench_linux_amd64 -o ./wrench && chmod +x ./wrench in the container didn't work because the Assets of release tag didn't include such the binary wrench_linux_amd64.

Context (Environment)

• wrench version: v1.2.0 and v1.2.1

WHAT

Could you add windows binary in the release?

WHY

I am using windows at work, it would be nice to use wrench there.

WHAT

• Preserve comments for schema.sql
  • when execute migrations to DB, wrench should strip comments in the schema file.
  • when load schema from DB, wrench should preserve comments and then apply comments to the schema file.

WHY

• would like to write comments to the schema file although Spanner does not recognize comments.

Expected Behavior

Check the error more rigorously.

Current Behavior

Wrench currently simply ignores an error in EnsureMigrationTable, but in some cases (for example, a Job does not have permission to access the table), it should return the error instead of ignoring it.

WHAT

Hi, is there support for using wrench with the local Cloud Spanner emulator? So far, I haven't been able to make it work. I've tried the following:

Shell session 1

$ gcloud beta emulators spanner start
Executing: docker run -p 127.0.0.1:9010:9010 -p 127.0.0.1:9020:9020 gcr.io/cloud-spanner-emulator/emulator:0.7.3
[cloud-spanner-emulator] 2020/06/12 21:59:57 gateway.go:135: Cloud Spanner emulator running.
[cloud-spanner-emulator] 2020/06/12 21:59:57 gateway.go:136: REST server listening at 0.0.0.0:9020
[cloud-spanner-emulator] 2020/06/12 21:59:57 gateway.go:137: gRPC server listening at 0.0.0.0:9010

Shell session 2

$  $(gcloud beta emulators spanner env-init)  # sets SPANNER_EMULATOR_HOST
$ ./wrench_darwin_amd64 create --directory . --project my-project --instance my-instance --database my-database
Error command: create, version: 1.0.0
	Failed to execute the operation to Cloud Spanner, Either caller is missing IAM permission spanner.databases.create on resource projects/my-project/instances/my-instance, or the CreateDatabaseRequest.create_statement field is malformed and the database name could not be identified to verify Cloud IAM Conditions.

It looks like wrench isn't picking up on the SPANNER_EMULATOR_HOST environment variable, and therefore is failing when hitting the actual Cloud Spanner API only to find that my dummy project / instance / database doesn't exist.

I'm guessing wrench just uses the Go client library for Cloud Spanner – which according to the emulator docs should honor the SPANNER_EMULATOR_HOST environment variable. So I might be missing something that's needed to make this work.

WHY

We rely on the Cloud Spanner emulator for local development and are looking for a good tool to manage schema migrations. It would be great if we could start using wrench for both local and Cloud development.

Thanks in advance.

EDIT

Perhaps googleapis/google-cloud-go#1602 is related?

WHAT

• make wrench respect SPANNER_EMULATOR_HOST

WHY

• to use wrench for the emulator as well.

Expected Behavior

Create/migration contains ALTER CHANGE STREAMS name SET FOR ... should succeed as per Spanner DDL docs

Current Behavior

Fails with errors like:

Error command: create, version: v1.5.0
	Failed to execute the operation to Cloud Spanner, schema.sql:9: got "\"FOR\"", expected OPTIONS

Steps to Reproduce

1. Prepare file schema.sql, contents is below:

CREATE TABLE User (
  ID STRING(MAX) NOT NULL,
  Name STRING(MAX) NOT NULL,
  Age INT64 NOT NULL
) PRIMARY KEY(ID);

CREATE CHANGE STREAM UserStream FOR User(Name);

ALTER CHANGE STREAM UserStream SET FOR User(Name, Age);

2. Run wrench create

Context (Environment)

• wrench version: 1.5.0

Expected Behavior

Creating a migration file to ALTER INDEX should succeed as per Spanner DDL docs

Current Behavior

Fails with errors like:

Error command: up, version: unknown
        failed to parse DDL/DML statements: <file>.sql:1.0: unknown DDL statement, <file>.sql:1.0: unknown DML statement

Steps to Reproduce

1. Start with schema file like ./schema/schema.sql:

CREATE TABLE players (
  playerUUID STRING(36) NOT NULL,
  player_name STRING(64) NOT NULL,
  email STRING(MAX) NOT NULL,
  password_hash BYTES(60) NOT NULL,  
  is_logged_in BOOL,
) PRIMARY KEY(playerUUID);

CREATE UNIQUE INDEX PlayerAuthentication ON players(email) STORING (password_hash);

2. Create the database using wrench:

wrench create --directory ./schema

3. Create a new migration file

wrench migrate create --directory ./schema

# cat ./schema/migrations/000001.sql
ALTER INDEX PlayerAuthentication ADD STORED COLUMN is_logged_in;

4. Run migration:

wrench migrate up --directory ./schema

Context (Environment)

• wrench version: 1.3.3

The workaround is to drop index and recreate. But it'd be nice to have ALTER INDEX supported.

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:

   wrench/pkg/spanner/client.go

   Line 134 in 9f346f2

   if t.TableName == "SchemaMigrations" {

2. cmd/migrate.go also hardcodes the migration table name:

   wrench/cmd/migrate.go

   Line 36 in 9f346f2

   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:

Expected Behavior

make build should run ok

Current Behavior

make build
bin/bazelisk build //:wrench
2022/04/27 18:44:31 could not download Bazel: could not determine filename to use for Bazel binary: unsupported machine architecture "arm64", must be x86_64
make: *** [build] Error 1

Steps to Reproduce

1. make build

Context (Environment)

Use a MacBook Pro M1

• wrench version: master

I cannot build wrench, probably bin/bazelisk need to be updated as well as the bazel version.

Expected Behavior

When a table is created with a foreign key constraint and no foreign key action is provided, wrench should not add ON DELETE NO ACTION to the create table statement.

Current Behavior

Wrench adds ON DELETE NO ACTION, changing the statement syntax.

Steps to Reproduce

1. Create 2 migration scripts
   script 1:

   CREATE TABLE Table1 (
     Table1ID STRING(MAX) NOT NULL,
   ) PRIMARY KEY(Table1ID);
   

   script 2:

   CREATE TABLE Table2 (
       Table2ID STRING(MAX) NOT NULL,
       Table1ID STRING(MAX) NOT NULL,
       CONSTRAINT FK_Table1Table2 FOREIGN KEY(Table1ID) REFERENCES Table1(Table1ID)
   ) PRIMARY KEY(Table2ID);
   

2. Create a local spanner instance with cloud spanner emulator
3. Apply migrations to cloud spanner emulator using wrench.
4. Second migration fails with the following error:

   Foreign key referential action ON DELETE NO ACTION is not supported.
   

   This error demonstrates that ON DELETE NO ACTION is being added by wrench. If it wasn't being added by wrench, then second migration would succeed.

Context (Environment)

• wrench version: v1.6.0
• possibly caused by dependency cloud.google.com/go/spanner upgrade from v1.47.0 to v1.49.0
  • I can reproduce with wrench v1.5.0 if I upgrade cloud.google.com/go/spanner to v1.49.0.

This change breaks my local and CI environments that use the emulator:

• The emulator does not accept create table statements with foreign key actions
• I cannot remove the foreign key action, since it's being added by wrench.

I have already created an issue in cloud spanner emulator: GoogleCloudPlatform/cloud-spanner-emulator#142 but I was told the fix should be on the wrench side, not the emulator side.

WHAT

There is a motivation to replace spansql with a more primitive implementation.
I want to discuss it.

WHY

Currently, wrench can't process statements which are not supported by spansql because of the spansql dependency.

spansql is used in ddlToStatements, dmlToStatements.
https://github.com/cloudspannerecosystem/wrench/blob/v1.4.1/pkg/spanner/migration.go#L144-L170

spansql is not actively maintained by Google and not 100% compatible with Cloud Spanner DDL/DML so the dependency on spansql makes it complex for wrench to support the new features of Cloud Spanner.

Reference

• Cloud Spanner Standard SQL lexical structure
  • https://cloud.google.com/spanner/docs/reference/standard-sql/lexical?hl=en
  • We should only think about comments and literals.
• spansql is introduced to support DDLs/DMLs with comments.
  • #54
  • Comment preserve is not yet implemented #15
• spanner-cli implements statement separator with comment strip
  • cloudspannerecosystem/spanner-cli#33
  • cloudspannerecosystem/spanner-cli#34
    • I think wrench can reuse this implementation.

Expected Behavior

Creating a migration file using GENERATE_UUID() should be successful.
The use of GENERATE_UUID() should be available.
https://cloud.google.com/spanner/docs/primary-key-default-value#universally_unique_identifier_uuid

Current Behavior

Fails with errors like:

Error command: up, version: 1.6.0
        failed to parse DDL/DML statements: 000001.sql:2: got "(" while expecting ")", 000001.sql:1.0: unknown DML statement

Steps to Reproduce

1. Create the database using wrench:

wrench create --directory ./schema

2. Create a new migration file:

wrench migrate create --directory ./schema

# cat ./schema/migrations/000001.sql
CREATE TABLE test (
    test_id STRING(36) NOT NULL DEFAULT (GENERATE_UUID())
);

3. Run migration:

wrench migrate up --directory ./schema

Context (Environment)

• wrench version: 1.6.0

WHAT

Implement command execution confirmation for commands that involve destructive operations, particularly wrench drop.
Introduce a -q/--quiet flag to facilitate execution without the confirmation prompt.

WHY

I accidentally executed a destructive change using wrench drop. It would be less error-prone and safer to include a confirmation step that verifies which database is to be dropped.

WHAT

Docker Hub release is stuck at 1.0.4.
https://hub.docker.com/r/mercari/wrench/tags

Will this be part of the release process going forward?

WHY

because the README points there

WHAT

• Divide and apply long DML which overs limit (20,000)
  • Cloud Spanner does not allow DML which overs limit.

WHY

• Currently if we would like to apply DML overs the limit, we must divide DML into some partials and it is tedious.

Expected Behavior

The value specified by either flag --project or env var SPANNER_PROJECT_ID is used.

Current Behavior

The value of the default configuration in gcloud is used.

This was discovered because the default project I have set does not have Cloud Spanner API enabled.

Steps to Reproduce

1. go install within cloned repo at v1.0.3
2. setup a default configuration in gcloud that does not have cloud spanner api enabled
3. try to override project: wrench --project ...
4. error

Context (Environment)

• wrench version: v1.0.3
• go version: go1.14.2

Currently wrench only supports partitioned DML migration files. But this excludes INSERT statements which is useful for seeding databases with static data.

Expected Behavior

If there are more than one files starting with the same number in their names then they should be either processed individually or reported as an error. Silently pretending as if there were only one of them is pretty misleading.

If there are two files with the same number in their names then either process all of them or report an error saying "File names must have different numbers in their names."

Current Behavior

Wrench executes only one of these files and ignores the rest.

Steps to Reproduce

1. Create two migration scripts and name them with the same number. For example, 20210831_123456_first.sql and 20210831_123456_second.sql
2. Define a CREATE TABLE first(...) in the first one and CREATE TABLE second(...) in the second one.
3. Do a migrate up
4. See your database. Table first will be there, table second will not.

WHAT

Make it possible to pass the Emulator host in the Config.

// example
cfg := wrenchSpanner.Config{
	Project:  "project",
	Instance: "instance",
	Database: "database",
	EmulatorHost: "localhost:9091",
}
spannerCli, err := wrenchSpanner.NewClient(ctx, &cfg)

WHY

Currently if we want to define an emulator host, we need to set the SPANNER_EMULATOR_HOST. Those who using the tool as library it is pretty hard to maintain, because we need to call os.Setenv, which is a pretty bad practices in case of testing.

Edit, in case of approval, I will be happy to work on it.

Expected Behavior

wrench can apply DML including TIMESTAMP_SUB function.

Current Behavior

It seems that wrench 1.1.0 fails to parse DML including TIMESTAMP_SUB.
wrench 1.0.4 can apply such DML correctly.

Steps to Reproduce

The following reproduction steps assumes bash or zsh.

# $PROJECT, $INSTANCE, $DATABASE should be set properly

# setup
$ wrench apply --project $PROJECT --instance $INSTANCE --database $DATABASE --ddl <(echo "CREATE TABLE ParsingTimestampSub (CreatedAt TIMESTAMP NOT NULL) PRIMARY KEY (CreatedAt);")

# OK case
$ wrench apply --project $PROJECT --instance $INSTANCE --database $DATABASE --dml <(echo "DELETE FROM ParsingTimestampSub WHERE CreatedAt < CURRENT_TIMESTAMP();")
0 rows affected.

# error case
$ wrench apply --project $PROJECT --instance $INSTANCE --database $DATABASE --dml <(echo "DELETE FROM ParsingTimestampSub WHERE CreatedAt < TIMESTAMP_SUB(CURRENT_TIMESTAMP(), INTERVAL 31 DAY);")
Error command: apply, version: unknown
        /proc/self/fd/11:1.94: got "31", want ")" or ","

# cleanup
$ wrench apply --project $PROJECT --instance $INSTANCE --database $DATABASE --ddl <(echo "DROP TABLE ParsingTimestampSub;")

Context (Environment)

• wrench version: 1.1.0

Expected Behavior

When specifying project, instance and database by command line args, I want wrench to use the values instead of environment variables.

Current Behavior

wrench use environment variables even if I pass project, instance and database by the command line args.

Steps to Reproduce

$ echo $SPANNER_PROJECT_ID                                                                                                                              
project-local-emulator

$ wrench apply --project my-gcp-project --instance instance1 --database database1 --dml ./sql/dml/000001_init.sql

Error command: apply, version: unknown
        Failed to execute the operation to Cloud Spanner, context deadline exceeded

$ unset SPANNER_PROJECT_ID                                                                                                               
$ echo $SPANNER_PROJECT_ID                                                                                                                              

$ wrench apply --project my-gcp-project --instance instance1 --database database1 --dml ./sql/dml/000001_init.sql
1 rows affected.

Context (Environment)

• wrench version: 1.3.0

wrench help says project, instance and database can be specified by command line args. For example, in the description of the arg for database, (optional. if not set, will use $SPANNER_DATABASE_ID value) seems to mean command line args have higher priority than environment variables.

Hello.

• There is no binary attached to release page. And also no 1.1.0 image is published to docker hub.
• Version number in https://github.com/cloudspannerecosystem/wrench/blob/v1.1.0/BUILD.bazel#L1 is still 1.0.4.

What

Similar to FlywayDB outOfOrder option.

Allows migrations to be run "out of order".
If you already have versions 1 and 3 applied, and now a version 2 is found, it will be applied too instead of being ignored.

Why

Flexibility to apply a hotfix to production, then cherry-pick the migration back to development environments which are already ahead of production.

WHAT

Make it so it's possible to instantiate the client in wrench/pkg/spanner with option.ClientOption - perhaps like this:

func NewClient(ctx context.Context, config *Config, opts ...option.ClientOption) (*Client, error)

WHY

I'm trying to use spannertest for unit testing and would like to perform a wrench migration on a new inmem fake db instance. However, I'd also like to avoid using SPANNER_EMULATOR_HOST as I'd like to have multiple instances of spannertest running in the unit test suite.

It is the case that a normal spanner Client can be instantiated such that it uses the spannertest server by passing option.WithGRPCConn(conn) to spanner.NewClient with conn being a gRPC connection to the spannertest server. Perhaps the wrench NewClient could also be able to interact with a spannertest server by instantiating the spanner.Client and the apiv1.DatabaseAdminClient with this option?