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 support for nomulus tool to connect to Cloud SQL #303

Merged
merged 1 commit into from Oct 9, 2019

Conversation

@hstonec
Copy link
Collaborator

commented Oct 7, 2019

This change is Reviewable

@googlebot googlebot added the cla: yes label Oct 7, 2019
@hstonec hstonec requested review from CydeWeys and weiminyu Oct 7, 2019
Copy link
Collaborator

left a comment

Reviewable status: 0 of 6 files reviewed, 4 unresolved discussions (waiting on @CydeWeys, @hstonec, and @weiminyu)


core/src/main/java/google/registry/persistence/PersistenceModule.java, line 83 at r1 (raw file):

  @Provides
  @AppEngineJpaTm
  public static JpaTransactionManager providesAppEngineJpaTm(

Should this be singleton?


core/src/main/java/google/registry/persistence/PersistenceModule.java, line 106 at r1 (raw file):

  @Provides
  @NomulusToolJpaTm
  public static JpaTransactionManager providesNomulusToolJpaTm(

Same.


core/src/main/java/google/registry/persistence/PersistenceModule.java, line 159 at r1 (raw file):

@Retention(RetentionPolicy.RUNTIME)

nit: I'm not sure if it is worth adding Retention. Default retention works for Dagger. And this code is unlikely to be shareable with other injection frameworks.


core/src/main/java/google/registry/tools/CommandWithCloudSql.java, line 23 at r1 (raw file):

Command

Why not extend CommandWithRemoteApi. SQL commands would need it anyway.

@hstonec hstonec force-pushed the hstonec:enable-tool-connect-to-cloud-sql branch from 1d8f263 to 15cb6fe Oct 8, 2019
Copy link
Collaborator Author

left a comment

Reviewable status: 0 of 6 files reviewed, 4 unresolved discussions (waiting on @CydeWeys and @weiminyu)


core/src/main/java/google/registry/persistence/PersistenceModule.java, line 83 at r1 (raw file):

Previously, weiminyu (Weimin Yu) wrote…

Should this be singleton?

Done.


core/src/main/java/google/registry/persistence/PersistenceModule.java, line 106 at r1 (raw file):

Previously, weiminyu (Weimin Yu) wrote…

Same.

Done.


core/src/main/java/google/registry/persistence/PersistenceModule.java, line 159 at r1 (raw file):

Previously, weiminyu (Weimin Yu) wrote…
@Retention(RetentionPolicy.RUNTIME)

nit: I'm not sure if it is worth adding Retention. Default retention works for Dagger. And this code is unlikely to be shareable with other injection frameworks.

hmm, removed them. The default retention policy is actually RUNTIME.


core/src/main/java/google/registry/tools/CommandWithCloudSql.java, line 23 at r1 (raw file):

Previously, weiminyu (Weimin Yu) wrote…
Command

Why not extend CommandWithRemoteApi. SQL commands would need it anyway.

Done.

@hstonec hstonec force-pushed the hstonec:enable-tool-connect-to-cloud-sql branch from 15cb6fe to db8bc9a Oct 8, 2019
Copy link
Collaborator

left a comment

Reviewable status: 0 of 6 files reviewed, 1 unresolved discussion (waiting on @CydeWeys and @weiminyu)

@hstonec hstonec merged commit a694e24 into google:master Oct 9, 2019
4 of 7 checks passed
4 of 7 checks passed
code-review/reviewable 6 files, 1 discussion left (CydeWeys, weiminyu)
Details
LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No code changes detected
Details
LGTM analysis: Java No new or fixed alerts
Details
cla/google All necessary CLAs are signed
kokoro-foss Kokoro build finished
Details
kokoro-internal Kokoro build finished
Details
@hstonec hstonec deleted the hstonec:enable-tool-connect-to-cloud-sql branch Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.