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

Add tool library for updating Spanner schema using Liquibase. #109

Merged
merged 1 commit into from May 11, 2022

Conversation

SanjayVas
Copy link
Member

@SanjayVas SanjayVas commented May 2, 2022

@SanjayVas SanjayVas force-pushed the sanjayvas-update-schema branch 2 times, most recently from 41e0135 to ded8a3d Compare May 3, 2022 00:41
@SanjayVas SanjayVas force-pushed the sanjayvas-update-schema branch 5 times, most recently from 518b338 to 90afdf5 Compare May 6, 2022 01:09
@SanjayVas SanjayVas force-pushed the sanjayvas-update-schema branch 2 times, most recently from 05c7d84 to d579bd6 Compare May 6, 2022 17:56
@SanjayVas SanjayVas requested review from stevenwarejones and efoxepstein and removed request for stevenwarejones May 6, 2022 17:57
@SanjayVas SanjayVas marked this pull request as ready for review May 6, 2022 17:57
Copy link
Contributor

@efoxepstein efoxepstein left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @SanjayVas and @stevenwarejones)


src/main/kotlin/org/wfanet/measurement/gcloud/spanner/tools/UpdateSchema.kt line 47 at r1 (raw file):

    val connectionString =
      if (flags.emulatorHost == null) {
        "jdbc:cloudspanner:/projects/${flags.projectName}/instances/${flags.instanceName}/" +

Maybe val databasePath = "projects/.../instances/.../databases/..." so you can reuse it between the two branches here (and also make it more compact).

That could even be a property on SpannerFlags.

Copy link
Member Author

@SanjayVas SanjayVas 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 5 files reviewed, 1 unresolved discussion (waiting on @efoxepstein and @stevenwarejones)


src/main/kotlin/org/wfanet/measurement/gcloud/spanner/tools/UpdateSchema.kt line 47 at r1 (raw file):

Previously, efoxepstein (Eli Fox-Epstein) wrote…

Maybe val databasePath = "projects/.../instances/.../databases/..." so you can reuse it between the two branches here (and also make it more compact).

That could even be a property on SpannerFlags.

Done, and moved to SpannerFlags.

Copy link
Contributor

@stevenwarejones stevenwarejones 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 5 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @efoxepstein)

@SanjayVas SanjayVas force-pushed the sanjayvas-update-schema branch 2 times, most recently from 0d27c21 to dfaff9a Compare May 10, 2022 19:51
Base automatically changed from sanjayvas-liquibase to main May 10, 2022 19:54
Copy link
Contributor

@stevenwarejones stevenwarejones left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @efoxepstein)

Copy link
Contributor

@efoxepstein efoxepstein 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 2 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @SanjayVas)

@SanjayVas SanjayVas merged commit c85914c into main May 11, 2022
@SanjayVas SanjayVas deleted the sanjayvas-update-schema branch May 11, 2022 17:58
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.

None yet

3 participants