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

Use Flyway to deploy SQL schema to non-prod #255

Merged
merged 6 commits into from
Sep 6, 2019

Conversation

weiminyu
Copy link
Collaborator

@weiminyu weiminyu commented Sep 4, 2019

Added Gradle tasks to deploy and drop schema in alpha
using Flyway.

Updated ClaimsList.java so that Hibernate-generated
schema would use the right types.

Using 'varchar(255)' instead of 'text' for string columns
for now. We will need to investigate how to force Hibernate
to use the desired types in all cases.Added Gradle tasks to deploy and drop schema in alpha
using Flyway.

Updated ClaimsList.java so that Hibernate-generated
schema would use the right types.

Using 'varchar(255)' instead of 'text' for string columns
for now. We will need to investigate how to force Hibernate
to use the desired types in all cases.


This change is Reviewable

Copy link
Member

@mindhog mindhog left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 10 files at r1.
Reviewable status: 4 of 10 files reviewed, 4 unresolved discussions (waiting on @CydeWeys, @gbrodman, @hstonec, @jianglai, @mindhog, and @weiminyu)


db/build.gradle, line 28 at r1 (raw file):

      .findOptionalProperty(dbServerProperty).orElse('localhost:5432')
  def dbName = rootProject.ext
      .findOptionalProperty(dbNameProperty).orElse('postgres')

Instead of making it optional, can we add this to gradle.properties instead? It would be helpful to have all of our properties in one place instead of strewn around the build files.


db/build.gradle, line 42 at r1 (raw file):

    def dbUser = cred[1]
    def dbPassword = cred[2]
    return ImmutableList.of(

Instead of doing these as immutable lists, you could do:
return [url: ..., user: dbUser, password: dbPassword]

and then access them as:

accessInfo.url
accessInfo.user
accessInfo.password

below


db/build.gradle, line 43 at r1 (raw file):

    def dbPassword = cred[2]
    return ImmutableList.of(
        "jdbc:postgresql://google/${dbName}?cloudSqlInstance=${sqlInstance}&socketFactory=com.google.cloud.sql.postgres.SocketFactory",

Please reformat to at most 100 chars.


db/build.gradle, line 61 at r1 (raw file):

  // - env: alpha, crash, sandbox, or production
  // - role:
  //   * superuser

Formatting of the comments seems weird. I assume "superuser" is the only legitimate value? If so, why not make it a default?

Copy link
Member

@CydeWeys CydeWeys left a comment

Choose a reason for hiding this comment

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

We need to get the TEXT fields working, otherwise we're going to start off with storing bad data in an incorrect format, and need yet another delta later on just to fix the field type.

Reviewable status: 4 of 10 files reviewed, 4 unresolved discussions (waiting on @CydeWeys, @gbrodman, @hstonec, @jianglai, and @weiminyu)

Copy link
Member

@CydeWeys CydeWeys left a comment

Choose a reason for hiding this comment

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

How about @column(columnDefinition = "text") ?

Reviewable status: 4 of 10 files reviewed, 4 unresolved discussions (waiting on @gbrodman, @hstonec, @jianglai, and @weiminyu)

Copy link
Member

@CydeWeys CydeWeys left a comment

Choose a reason for hiding this comment

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

We might also make a simple type translator to do this, that would allow a little bit better abstraction in case someone in the future wants to support other DBs.

E.g. something like @text String columnName;

Where @text is an annotation we've defined that turns it into a TEXT field. Then add a presubmit on the .sql schema files ensuring that we never add a VARCHAR and we're good.

Reviewable status: 4 of 10 files reviewed, 4 unresolved discussions (waiting on @gbrodman, @hstonec, @jianglai, and @weiminyu)

Copy link
Collaborator Author

@weiminyu weiminyu left a comment

Choose a reason for hiding this comment

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

I'm using a hibernate custom dialect to do wholesale mappings:
varchar -> text
timestamp -> timestamptz

This is only needed in GenerateSqlSchemaCommand, and is safe to tinker with anyway we want.

Reviewable status: 3 of 12 files reviewed, 4 unresolved discussions (waiting on @gbrodman, @hstonec, @jianglai, and @mindhog)


db/build.gradle, line 28 at r1 (raw file):

Previously, mindhog (Michael Muller) wrote…

Instead of making it optional, can we add this to gradle.properties instead? It would be helpful to have all of our properties in one place instead of strewn around the build files.

Done.


db/build.gradle, line 42 at r1 (raw file):

Previously, mindhog (Michael Muller) wrote…

Instead of doing these as immutable lists, you could do:
return [url: ..., user: dbUser, password: dbPassword]

and then access them as:

accessInfo.url
accessInfo.user
accessInfo.password

below

Done.


db/build.gradle, line 43 at r1 (raw file):

Previously, mindhog (Michael Muller) wrote…

Please reformat to at most 100 chars.

Done.


db/build.gradle, line 61 at r1 (raw file):

Previously, mindhog (Michael Muller) wrote…

Formatting of the comments seems weird. I assume "superuser" is the only legitimate value? If so, why not make it a default?

Will add more roles, and default will be 'reader'.

Copy link
Contributor

@hstonec hstonec left a comment

Choose a reason for hiding this comment

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

Reviewable status: 3 of 12 files reviewed, 5 unresolved discussions (waiting on @CydeWeys, @gbrodman, @hstonec, @jianglai, @mindhog, and @weiminyu)


db/build.gradle, line 72 at r3 (raw file):

           --key sql-credentials-on-gcs-key --plaintext-file=- \
           --ciphertext-file=- \
           --project=domain-registry${env}-keys"""

Why do we store the keyring in another project?

Copy link
Member

@mindhog mindhog left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 10 files at r1, 6 of 8 files at r2, 1 of 1 files at r3.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @CydeWeys, @gbrodman, @jianglai, @mindhog, and @weiminyu)


core/src/main/java/google/registry/tools/GenerateSqlSchemaCommand.java, line 192 at r3 (raw file):

      registerColumnType(Types.VARCHAR, "text");
      registerColumnType(Types.TIMESTAMP, "timestamptz");
    }

Please regenerate the generated schema:

java -jar ./core/build/libs/nomulus.jar -e alpha generate_sql_schema -o db/src/main/resources/sql/schema/db-schema.sql.generated


db/build.gradle, line 32 at r3 (raw file):

        'url': "jdbc:postgresql://${hostAndPort}/${dbName}",
        'user': findProperty('dbUser'),
        'password': findProperty('dbPassword')]

Note that you shouldn't need to quote the keys.


db/src/main/resources/sql/flyway/V1__new_claims_list_and_entry.sql, line 15 at r3 (raw file):

-- limitations under the License.

    create table "ClaimsEntry" (

Do the double-quotes have the same semantics as the back-quotes?


db/src/main/resources/sql/schema/nomulus.golden.sql, line 1 at r3 (raw file):

What's the intended purpose of this file?

Copy link
Contributor

@hstonec hstonec left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @CydeWeys, @gbrodman, @jianglai, @mindhog, and @weiminyu)


db/build.gradle, line 72 at r3 (raw file):

Previously, hstonec (Shicong Huang) wrote…

Why do we store the keyring in another project?

ok, I noticed that we already have a section of kms in the yaml file with exact same set up. But I was wondering if it is ok to expose these information to the external? Right now, the keyring name, project id are only stored in the internal file.

Added Gradle tasks to deploy and drop schema in alpha
using Flyway.

Updated ClaimsList.java so that Hibernate-generated
schema would use the right types.

Using 'varchar(255)' instead of 'text' for string columns
for now. We will need to investigate how to force Hibernate
to use the desired types in all cases.
Added Gradle tasks to deploy and drop schema in alpha
using Flyway.

Updated ClaimsList.java so that Hibernate-generated
schema would use the right types.

Using 'varchar(255)' instead of 'text' for string columns
for now. We will need to investigate how to force Hibernate
to use the desired types in all cases.Added Gradle tasks to deploy and drop schema in alpha
using Flyway.

Updated ClaimsList.java so that Hibernate-generated
schema would use the right types.

Using 'varchar(255)' instead of 'text' for string columns
for now. We will need to investigate how to force Hibernate
to use the desired types in all cases.
Added Gradle tasks to deploy and drop schema in alpha
using Flyway.

Corrected the type of ClaimsEntry's revision_id column.
It should be plain int8, not bigserial.

Make GenerateSqlSchemaCommand use a custom dialect that
converts all varchar type to 'text' and timestamp to
'timestamptz'.
Added Gradle tasks to deploy and drop schema in alpha
using Flyway.

Use a custome dialect in GenerateSqlSchemaCommand to
convert varchar type to 'text' and timestamp to 'timestamptz'.

Corrected ClaimsEntry's revision_id column type to int8.
This column tracks parent table's primary key and should
not be bigserial.
Added Gradle tasks to deploy and drop schema in alpha
using Flyway.

Use a custome dialect in GenerateSqlSchemaCommand to
convert varchar type to 'text' and timestamp to 'timestamptz'.

Corrected ClaimsEntry's revision_id column type to int8.
This column tracks parent table's primary key and should
not be bigserial.
Added Gradle tasks to deploy and drop schema in alpha
using Flyway.

Use a custome dialect in GenerateSqlSchemaCommand to
convert varchar type to 'text' and timestamp to 'timestamptz'.

Corrected ClaimsEntry's revision_id column type to int8.
This column tracks parent table's primary key and should
not be bigserial.
Copy link
Collaborator Author

@weiminyu weiminyu left a comment

Choose a reason for hiding this comment

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

Reviewable status: 10 of 13 files reviewed, 8 unresolved discussions (waiting on @CydeWeys, @gbrodman, @hstonec, @jianglai, @mindhog, and @weiminyu)


core/src/main/java/google/registry/tools/GenerateSqlSchemaCommand.java, line 192 at r3 (raw file):

Previously, mindhog (Michael Muller) wrote…

Please regenerate the generated schema:

java -jar ./core/build/libs/nomulus.jar -e alpha generate_sql_schema -o db/src/main/resources/sql/schema/db-schema.sql.generated

Done.


db/build.gradle, line 72 at r3 (raw file):

Previously, hstonec (Shicong Huang) wrote…

ok, I noticed that we already have a section of kms in the yaml file with exact same set up. But I was wondering if it is ok to expose these information to the external? Right now, the keyring name, project id are only stored in the internal file.

I think we should continue keeping such information in internal config.


db/src/main/resources/sql/flyway/V1__new_claims_list_and_entry.sql, line 15 at r3 (raw file):

Previously, mindhog (Michael Muller) wrote…

Do the double-quotes have the same semantics as the back-quotes?

Back-quote is a mysql thing and is illegal for name quoting in postgresql


db/src/main/resources/sql/schema/nomulus.golden.sql, line 1 at r3 (raw file):

Previously, mindhog (Michael Muller) wrote…

What's the intended purpose of this file?

We will add test to verify that the final schema produced by the incremental scripts is equivalent to this schema.

Copy link
Member

@mindhog mindhog left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r4.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @CydeWeys, @gbrodman, @jianglai, and @weiminyu)

@weiminyu weiminyu merged commit 471ed7c into google:master Sep 6, 2019
@weiminyu weiminyu deleted the sql-from-desktop branch September 6, 2019 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants