Add support for comments in Spanner migrations #409

alethenorio · 2020-06-24T12:54:53Z

At the moment GCP Spanner backend does not seem to support comments (issue being tracked at https://issuetracker.google.com/issues/159730604).
By parsing the migration DDL with spansql we are able to support comments and remove them before applying on the database.

Includes tests for comments as well

coveralls · 2020-06-24T13:22:08Z

Pull Request Test Coverage Report for Build 800

10 of 27 (37.04%) changed or added relevant lines in 1 file are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage decreased (-0.2%) to 53.218%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
database/spanner/spanner.go	10	27	37.04%

Files with Coverage Reduction	New Missed Lines	%
database/spanner/spanner.go	1	7.01%

Totals
Change from base Build 795:	-0.2%
Covered Lines:	2613
Relevant Lines:	4910

💛 - Coveralls

alethenorio · 2020-06-24T13:39:05Z

What's the procedure here given the tiny coverage decrease due to a error check (which is already being tested on the library that returns the actual error anyway)?

dhui

Thanks for the PR!
Don't worry about the drop in test coverage since spanner doesn't have integration tests...

dhui · 2020-06-24T18:20:30Z

database/spanner/spanner.go

@@ -142,9 +143,12 @@ func (s *Spanner) Run(migration io.Reader) error {
 	}

 	// run migration
-	stmts := migrationStatements(migr)
-	ctx := context.Background()
+	stmts, err := migrationStatements(migr)


Make statement parsing optional and disabled by default.
migrate should not be parsing migrations since we treat the migration contents as an opaque blob

Not sure why this isn't already behind an optional. Let's name it something like x-clean-statements and put it in Config.CleanStatements

Alright. I assumed the way it was working before was not considered parsing so I restored it and put behind a flag. Felt that x-strip-comments is a little more representative of the feature. What do you think?

Or did you mean to take the incoming byte slice and pass it straight as one statement?

I believe ParseDDL() also supports multiple statements, so x-strip-comments isn't an accurate description of the behavior.
CleanStatements seems like be best name so far since it re-formats the statements. e.g. does more than just parsing and validating structure.

I'm ok with making this a backwards incompatible change since it's limited to the spanner db driver and the trade-off of cleaner code is worth it. So anyone using this updated driver will need to specify x-clean-statements to continue multi-statement support. We'll document this in the release notes as a breaking change.

Alright. Another push. Added a little more documentation on the README as well

Seems like it didn't end up getting listed as breaking, FWIW.

Running into some issues with this after updating, mentioned in #426

Thanks @wyardley! I've updated the v4.12.0 release notes to mention this breaking change.

dhui · 2020-06-24T18:24:11Z

@alethenorio if you have bandwidth, we can bump up the test coverage for spanner in a separate PR: #410

alethenorio · 2020-06-24T19:00:44Z

@alethenorio if you have bandwidth, we can bump up the test coverage for spanner in a separate PR: #410

No promises but I'll try to get to it

dhui

Thanks for the update!
Just a few minor things left.

dhui · 2020-06-26T04:55:55Z

database/spanner/spanner.go

+		}
+	} else {
+		// run migration
+		stmts = migrationStatements(migr)


Can you remove migrationStatements() since it's now replaced by cleanStatements()?
e.g. something along the lines of:

stmts := []string{string(migr)} if s.config.CleanStatements { stmts, err = cleanStatements(migr) if err != nil { return err } }

dhui · 2020-06-26T04:56:34Z

database/spanner/README.md

@@ -12,6 +12,7 @@ See [Google Spanner Documentation](https://cloud.google.com/spanner/docs) for de
 | Param | WithInstance Config | Description |
 | ----- | ------------------- | ----------- |
 | `x-migrations-table` | `MigrationsTable` | Name of the migrations table |
+| `x-clean-statements` | `CleanStatements` | Whether to parse DDL statements before running migration towards Spanner |


Change this to "Whether to parse and clean". Also add something in the description about this flag and multi statement support.

dhui · 2020-06-26T05:00:28Z

database/spanner/spanner.go

+	if err != nil {
+		return nil, err
+	}
+	stmts := []string{}


Nit: pre-allocate the slice e.g. make([]string, 0, len(ddl.List))

alethenorio · 2020-06-29T08:53:54Z

All very good comments. Fixed them all 👍

dhui

Thanks for addressing my feedback!
2 final things and we're good to merge.

dhui · 2020-06-30T00:10:48Z

database/spanner/spanner_test.go

-			if stmts := migrationStatements([]byte(tc.multiStatement)); !assert.Equal(t, stmts, tc.expected) {
+			stmts, err := cleanStatements([]byte(tc.multiStatement))
+			if err != nil {
+				t.Error()


Make this t.Fatal("Error cleaning statements:", err)

Fixed. Since we are already using testify I switched the whole if statement to use require.NoError which does the same but avoids the extra if. Let me know if this is an issue

Cool, didn't know about require.NoError()!

dhui · 2020-06-30T00:13:17Z

database/spanner/spanner_test.go

+			if err != nil {
+				t.Error()
+			}
+			if !assert.Equal(t, tc.expected, stmts) {


This if check isn't necessary: https://godoc.org/github.com/stretchr/testify/assert#hdr-Example_Usage

Doh, I've done this a million times before and probably missed this on copy paste

At the moment GCP Spanner backend does not seem to support comments (issue being tracked at https://issuetracker.google.com/issues/159730604). By adding support for parsing the migration DDL with spansql we are able to support comments and remove them before applying on the database

dhui

Thanks again for the PR!

dhui requested changes Jun 24, 2020

View reviewed changes

alethenorio force-pushed the spanner-comments branch from 85f3982 to e298c77 Compare June 24, 2020 18:59

alethenorio force-pushed the spanner-comments branch from e298c77 to 8c522e9 Compare June 25, 2020 07:42

dhui requested changes Jun 26, 2020

View reviewed changes

alethenorio force-pushed the spanner-comments branch from 8c522e9 to f7da555 Compare June 29, 2020 08:53

alethenorio requested a review from dhui June 29, 2020 08:54

dhui requested changes Jun 30, 2020

View reviewed changes

alethenorio force-pushed the spanner-comments branch from f7da555 to e6faa29 Compare June 30, 2020 07:25

dhui approved these changes Jun 30, 2020

View reviewed changes

dhui merged commit 8eb1d30 into golang-migrate:master Jun 30, 2020

wyardley mentioned this pull request Aug 15, 2020

setting x-clean-statements for spanner #425

Closed

renovate bot mentioned this pull request May 28, 2021

fix(deps): update module github.com/golang-migrate/migrate to v4 - autoclosed advenjourney/advenjourney#20

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for comments in Spanner migrations #409

Add support for comments in Spanner migrations #409

alethenorio commented Jun 24, 2020

coveralls commented Jun 24, 2020 •

edited

Loading

alethenorio commented Jun 24, 2020

dhui left a comment

dhui Jun 24, 2020

alethenorio Jun 24, 2020 •

edited

Loading

dhui Jun 24, 2020

alethenorio Jun 25, 2020

wyardley Aug 15, 2020

dhui Aug 16, 2020

dhui commented Jun 24, 2020

alethenorio commented Jun 24, 2020

dhui left a comment

dhui Jun 26, 2020

alethenorio Jun 29, 2020

dhui Jun 26, 2020

alethenorio Jun 29, 2020

dhui Jun 26, 2020

alethenorio Jun 29, 2020

alethenorio commented Jun 29, 2020

dhui left a comment

dhui Jun 30, 2020

alethenorio Jun 30, 2020

dhui Jun 30, 2020

dhui Jun 30, 2020

alethenorio Jun 30, 2020

alethenorio Jun 30, 2020

dhui left a comment

Add support for comments in Spanner migrations #409

Add support for comments in Spanner migrations #409

Conversation

alethenorio commented Jun 24, 2020

coveralls commented Jun 24, 2020 • edited Loading

Pull Request Test Coverage Report for Build 800

💛 - Coveralls

alethenorio commented Jun 24, 2020

dhui left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alethenorio Jun 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhui commented Jun 24, 2020

alethenorio commented Jun 24, 2020

dhui left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alethenorio commented Jun 29, 2020

dhui left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhui left a comment

Choose a reason for hiding this comment

coveralls commented Jun 24, 2020 •

edited

Loading

alethenorio Jun 24, 2020 •

edited

Loading