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

Make dev project configurable #371

Merged
merged 2 commits into from Nov 22, 2019
Merged

Make dev project configurable #371

merged 2 commits into from Nov 22, 2019

Conversation

@jianglai
Copy link
Member

jianglai commented Nov 15, 2019

We should not hardcode our dev project in the public config file.


This change is Reviewable

We should not hardcode our dev project in the public config file.
@jianglai jianglai requested a review from weiminyu Nov 15, 2019
@googlebot googlebot added the cla: yes label Nov 15, 2019
@jianglai

This comment has been minimized.

Copy link
Member Author

jianglai commented Nov 20, 2019

Ping?

@jianglai jianglai requested a review from mindhog Nov 22, 2019
Copy link
Member

mindhog left a comment

Reviewed 2 of 2 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jianglai and @weiminyu)


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

    def command =
        """gsutil cp \
           gs://${rootProject.ext.devProject}-deploy/cloudsql-credentials/${env}/${role}_credential.enc - | \

I don't think this will work. As I recall, we used the dev project for storing our credentials for all environments, and external users would have to do something similar.

@jianglai

This comment has been minimized.

Copy link
Member Author

jianglai commented Nov 22, 2019

Reviewed 2 of 2 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jianglai and @weiminyu)

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

    def command =
        """gsutil cp \
           gs://${rootProject.ext.devProject}-deploy/cloudsql-credentials/${env}/${role}_credential.enc - | \

I don't think this will work. As I recall, we used the dev project for storing our credentials for all environments, and external users would have to do something similar.

I don't quite understand your comment. Isn't this precisely the point why we want to make this variable configurable? There's a corresponding CR in the Gerrit that sets this value for our project.

Copy link
Member

mindhog left a comment

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jianglai and @weiminyu)


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

Previously, jianglai (Lai Jiang) wrote…

I don't quite understand your comment. Isn't this precisely the point why we want to make this variable configurable? There's a corresponding CR in the Gerrit that sets this value for our project.

My point was that it's not the same the application environment, but I clearly wasn't paying close enough attention: you use the devProject variable and the $devProject-deploy project.

My only remaining complaint is that this is not currently defined anywhere - though I think you have another PR out for that just now?

Copy link
Member

mindhog left a comment

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jianglai and @weiminyu)


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

Previously, mindhog (Michael Muller) wrote…

My point was that it's not the same the application environment, but I clearly wasn't paying close enough attention: you use the devProject variable and the $devProject-deploy project.

My only remaining complaint is that this is not currently defined anywhere - though I think you have another PR out for that just now?

Actually, NM that last statement, that was this PR whose title I was looking at :-). We should either define this in gradle.properties or somewhere else.

Also, the "ext" part is only necessary when defining the variable, not for using it.

They are only needed when defining properties.
@jianglai jianglai requested a review from mindhog Nov 22, 2019
Copy link
Member Author

jianglai left a comment

Reviewable status: 1 of 3 files reviewed, 1 unresolved discussion (waiting on @mindhog and @weiminyu)


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

Previously, mindhog (Michael Muller) wrote…

Actually, NM that last statement, that was this PR whose title I was looking at :-). We should either define this in gradle.properties or somewhere else.

Also, the "ext" part is only necessary when defining the variable, not for using it.

Removed "ext" in other places as well.

Copy link
Member

mindhog left a comment

Reviewed 1 of 2 files at r2.
Reviewable status: 2 of 3 files reviewed, all discussions resolved (waiting on @mindhog and @weiminyu)

@jianglai jianglai merged commit cc5f625 into google:master Nov 22, 2019
6 of 7 checks passed
6 of 7 checks passed
code-review/reviewable 1 file left (mindhog, 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
@jianglai jianglai deleted the jianglai:dev branch Nov 22, 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.