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

Improve error handling for environment property #295

Merged
merged 5 commits into from Oct 4, 2019

Conversation

@mindhog
Copy link
Member

commented Oct 1, 2019

Improve the error messages that we get for a bad or missing environment
property. Move the property processing into the main build so that we do it
only once and configure all of the appengine deployment tasks to check that a
project has been defined and print a friendly error if it hasn't.

Note that even if the check isn't configured, this change prevents deployment
because the gcpProject will be set to null. It just won't print as useful an
error message.

Tested: ran appengineDeployAll with and without the environment property, ran
"build" to verify that we don't get any complaints for non-deployment targets.


This change is Reviewable

Improve the error messages that we get for a bad or missing environment
property.  Move the property processing into the main build so that we do it
only once and configure all of the appengine deployment tasks to check that a
project has been defined and print a friendly error if it hasn't.

Note that even if the check isn't configured, this change prevents deployment
because the gcpProject will be set to null.  It just won't print as useful an
error message.

Tested: ran appengineDeployAll with and without the environment property, ran
"build" to verify that we don't get any complaints for non-deployment targets.
@mindhog mindhog requested review from jianglai and CydeWeys Oct 1, 2019
@googlebot googlebot added the cla: yes label Oct 1, 2019
Copy link
Member

left a comment

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


build.gradle, line 122 at r1 (raw file):

def environments = ['production', 'sandbox', 'alpha', 'crash']

def projects = ['production': 'domain-registry',

This configuration works for us, but won't work for any other user of nomulus. It needs some way to configure the project ID for each GCP project.

Copy link
Member

left a comment

Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @CydeWeys, @jianglai, and @mindhog)


build.gradle, line 127 at r1 (raw file):

                'crash'     : 'domain-registry-crash']

def environment = rootProject.findProperty("environment")

This should use single quotes for consistency with the other strings in this file.

@jianglai jianglai self-requested a review Oct 1, 2019
Copy link
Member

left a comment

I don't understand why do we want to move everything in projects.gradle into the main build file?

Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @jianglai and @mindhog)

@jianglai jianglai requested a review from CydeWeys Oct 1, 2019
Copy link
Member

left a comment

Reviewed 3 of 3 files at r1.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @CydeWeys and @mindhog)


build.gradle, line 127 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

This should use single quotes for consistency with the other strings in this file.

I think we can save the dictionary in a separate file and store a proprietary version of it on GoB.


build.gradle, line 137 at r1 (raw file):

Quoted 6 lines of code…
    // Print a prominent warning in bright red.
    System.err.println('\033[31;1m-----------------------------------------------------------------')
    System.err.println('*** DANGER WILL ROBINSON!')
    System.err.println('*** You may not deploy to production or sandbox from gradle.  Do a')
    System.err.println('*** release from Spinnaker, see deployment playbook.')
    System.err.println('-----------------------------------------------------------------')

This needs to go away, as I mentioned in the previous PR. We should only check it during deployment. This check happens too early and will prevent building the release candidates.


build.gradle, line 153 at r1 (raw file):

rootProject.ext.prodOrSandboxEnv = environment in ['production', 'sandbox']

My suggestion is make this boolean check both environment and gcpProject. We can then check this boolean in verifyDeployment.


build.gradle, line 157 at r1 (raw file):

// See projects.gradle.  If the user doesn't specify "environment gcp

projects.gradle is deleted...

Copy link
Member Author

left a comment

Because projects.gradle is reevaluated for every one of the subprojects, which means that if we are using it to assign "environment" and "gcpProject", the defaulting behavior does not function correctly for subsequent invocations because those variables are defined to their default values at that point.

Reviewable status: 2 of 3 files reviewed, 5 unresolved discussions (waiting on @CydeWeys and @jianglai)


build.gradle, line 122 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

This configuration works for us, but won't work for any other user of nomulus. It needs some way to configure the project ID for each GCP project.

That's a valid concern, but is out of scope for this PR. This code was moved from another file for reasons identified in the PR discussion.


build.gradle, line 127 at r1 (raw file):

Previously, jianglai (Lai Jiang) wrote…

I think we can save the dictionary in a separate file and store a proprietary version of it on GoB.

Added a TODO.


build.gradle, line 137 at r1 (raw file):

Previously, jianglai (Lai Jiang) wrote…
    // Print a prominent warning in bright red.
    System.err.println('\033[31;1m-----------------------------------------------------------------')
    System.err.println('*** DANGER WILL ROBINSON!')
    System.err.println('*** You may not deploy to production or sandbox from gradle.  Do a')
    System.err.println('*** release from Spinnaker, see deployment playbook.')
    System.err.println('-----------------------------------------------------------------')

This needs to go away, as I mentioned in the previous PR. We should only check it during deployment. This check happens too early and will prevent building the release candidates.

Yes, you're right. Fortunately, in this PR, there's a place to put it.


build.gradle, line 153 at r1 (raw file):

Previously, jianglai (Lai Jiang) wrote…
rootProject.ext.prodOrSandboxEnv = environment in ['production', 'sandbox']

My suggestion is make this boolean check both environment and gcpProject. We can then check this boolean in verifyDeployment.

I don't understand. As it stands, if environment is in sandbox or prod, gcpProject will always be null. Switched to using this boolean in the verify, though.


build.gradle, line 157 at r1 (raw file):

Previously, jianglai (Lai Jiang) wrote…
// See projects.gradle.  If the user doesn't specify "environment gcp

projects.gradle is deleted...

Done.

Copy link
Member

left a comment

But why can't we apply projects.gradle in build.gradle directly, instead of applying it from appengine_war.gradle? I still prefer to have logic specific to how to map environments to projects localized in a separate file.

Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @CydeWeys and @mindhog)


build.gradle, line 153 at r1 (raw file):

Previously, mindhog (Michael Muller) wrote…

I don't understand. As it stands, if environment is in sandbox or prod, gcpProject will always be null. Switched to using this boolean in the verify, though.

See my comment above.


build.gradle, line 136 at r2 (raw file):

crash

alpha would be better, wouldn't it? This allows us to stage and deploy to alpha (which is most common) without the need to supply a flag.


build.gradle, line 137 at r2 (raw file):

Quoted 6 lines of code…
} else if (environment != 'production' && environment != 'sandbox') {
    gcpProject = projects[environment]
    if (gcpProject == null) {
        throw new GradleException("-Penvironment must be one of ${environments}.")
    }
}

I don't understand how this if works. Why can't we just do:

  1. Find the environment system property.
  2. Set gcpProject from the directory using environment, fail if gcpProject is null (i. e. the environment is not recognized).
  3. Set a boolean to indicate if prod or sandbox is chosen.
  4. Check the boolean for all deployment tasks.

Here you only set gcpProject if environment is not alpha or crash, doesn't it still prevent us from staging prod or sandbox?

Also, I guess I fundamentally didn't fully understand how the accidental deployment could have happened. The code as it stands now does not take gcpProject from the command line, why is it that when you set -P gcpProject = domain-registry-alpha, it somehow affected the gcpProjectfield? You can see that in order to set environment from the command line, we need to call def environment = rootProject.findProperty("environment"). There is no such thing for gcpProject. So how can it be affected by the -P flag? Am I missing something here?

Copy link
Member Author

left a comment

I agree that the mapping should be in a separate file for easy overriding, but the functions that do the check should not be in that file as they're completely reusable.

Is you concern purely about being able to override the mapping? If so, I'm ok with breaking that out.

If you'd also like to modularize the support functions, I don't see why we should do that for this specific case when there are any number of other pieces of /build.gradle that could also be broken out. I'm personally in favor of leaving as much stuff in build.gradle as possible, at least until it becomes unwieldly, just because it's more discoverable there.

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @CydeWeys and @jianglai)


build.gradle, line 153 at r1 (raw file):

Previously, jianglai (Lai Jiang) wrote…

See my comment above.

Done.


build.gradle, line 136 at r2 (raw file):

Previously, jianglai (Lai Jiang) wrote…
crash

alpha would be better, wouldn't it? This allows us to stage and deploy to alpha (which is most common) without the need to supply a flag.

Done (sorry, I think I did that in the earlier incarnation and then lost it for this one)


build.gradle, line 137 at r2 (raw file):

I don't understand how this if works. Why can't we just do:

Because it's less safe. By having gcpProject be null, we guarantee failure of deployment even if there's a deployment task we haven't added the verification to. (I looked in the app engine plugin docs for a hook or some other way to ensure this, but couldn't find anything)

There is no such thing for gcpProject. So how can it be affected by the -P flag? Am I missing something here?

I think what you're missing is that properties are automatically translated into project scope variables in gradle :-) You could omit the "def environment = rootProject.findProperty(...)" for environment entirely (if it were defined in gradle.properties along with everything else, otherwise we'd always get an error if it were omitted). And, in fact, we should do that. So done :-)

Copy link
Member

left a comment

Yeah, mostly for overloading, I'm fine with leaving the functions in the main build.gradle file.

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @CydeWeys and @mindhog)


build.gradle, line 136 at r2 (raw file):

Previously, mindhog (Michael Muller) wrote…

Done (sorry, I think I did that in the earlier incarnation and then lost it for this one)

I think you forgot to push :D


build.gradle, line 137 at r2 (raw file):

Previously, mindhog (Michael Muller) wrote…

I don't understand how this if works. Why can't we just do:

Because it's less safe. By having gcpProject be null, we guarantee failure of deployment even if there's a deployment task we haven't added the verification to. (I looked in the app engine plugin docs for a hook or some other way to ensure this, but couldn't find anything)

There is no such thing for gcpProject. So how can it be affected by the -P flag? Am I missing something here?

I think what you're missing is that properties are automatically translated into project scope variables in gradle :-) You could omit the "def environment = rootProject.findProperty(...)" for environment entirely (if it were defined in gradle.properties along with everything else, otherwise we'd always get an error if it were omitted). And, in fact, we should do that. So done :-)

Ha, that's what I was missing!

Still my original point stands. We can not just set gcpProject to null for production and sandbox environment (which is what the current logic does). This prevents us from staging the files (so we will not be able to build the WAR files for prod and sandbox).

Copy link
Member Author

left a comment

Reviewable status: 2 of 4 files reviewed, 4 unresolved discussions (waiting on @CydeWeys and @jianglai)


build.gradle, line 136 at r2 (raw file):

Previously, jianglai (Lai Jiang) wrote…

I think you forgot to push :D

Ooops, sorry. You're right.


build.gradle, line 137 at r2 (raw file):

Previously, jianglai (Lai Jiang) wrote…

Ha, that's what I was missing!

Still my original point stands. We can not just set gcpProject to null for production and sandbox environment (which is what the current logic does). This prevents us from staging the files (so we will not be able to build the WAR files for prod and sandbox).

I believe that we can. I've verified that appengineStage still works (gcpProject only appears to be used in "deploy"). Is there any other task you'd like me to verify?

Copy link
Member

left a comment

Reviewable status: 2 of 4 files reviewed, 4 unresolved discussions (waiting on @CydeWeys, @jianglai, and @mindhog)


build.gradle, line 122 at r1 (raw file):

Previously, mindhog (Michael Muller) wrote…

That's a valid concern, but is out of scope for this PR. This code was moved from another file for reasons identified in the PR discussion.

What will happen to open source users that try to build/deploy using this codebase?

Also what's up with the -xxx?

@jianglai jianglai requested a review from CydeWeys Oct 4, 2019
Copy link
Member

left a comment

Reviewed 2 of 2 files at r3.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @CydeWeys and @mindhog)


appengine_war.gradle, line 68 at r3 (raw file):

        if (!rootProject.prodOrSandboxEnv) {
            version = 'GCLOUD_CONFIG'

Use gcpProject = null to check.


build.gradle, line 122 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

What will happen to open source users that try to build/deploy using this codebase?

Also what's up with the -xxx?

I presume the -xxx is pushed by mistake.


build.gradle, line 137 at r2 (raw file):

Previously, mindhog (Michael Muller) wrote…

I believe that we can. I've verified that appengineStage still works (gcpProject only appears to be used in "deploy"). Is there any other task you'd like me to verify?

Ah, OK I now understand. I initially thought that we need gcpProject to pull the appropriate configs, but we only need environment for that.


build.gradle, line 144 at r3 (raw file):

rootProject.ext.prodOrSandboxEnv = environment in ['production', 'sandbox']

OK I guess we don't need this anymore. gcpProject = null effectively does the same check.

Copy link
Member Author

left a comment

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


build.gradle, line 122 at r1 (raw file):

Previously, jianglai (Lai Jiang) wrote…

I presume the -xxx is pushed by mistake.

Yep. Better to suffer shame in the review than to push to prod while testing the code to prevent pushing to prod :-) Fixed.


build.gradle, line 137 at r2 (raw file):

Previously, jianglai (Lai Jiang) wrote…

Ah, OK I now understand. I initially thought that we need gcpProject to pull the appropriate configs, but we only need environment for that.

Done.


build.gradle, line 144 at r3 (raw file):

Previously, jianglai (Lai Jiang) wrote…
rootProject.ext.prodOrSandboxEnv = environment in ['production', 'sandbox']

OK I guess we don't need this anymore. gcpProject = null effectively does the same check.

Not quite:
gcpProject == null means that either the environment is not legal for deployment OR that the environment was not explicitly specified. I think we should explicitly require specifying an environment for "deploy", do you agree?

Copy link
Member Author

left a comment

Reviewable status: 3 of 4 files reviewed, 5 unresolved discussions (waiting on @CydeWeys and @jianglai)


appengine_war.gradle, line 68 at r3 (raw file):

Previously, jianglai (Lai Jiang) wrote…
        if (!rootProject.prodOrSandboxEnv) {
            version = 'GCLOUD_CONFIG'

Use gcpProject = null to check.

See previous comment.

Copy link
Member

left a comment

There are some follow up changes to do, like changing the release and CI pipeline to explicitly build for production. I'll make those changes after this PR is submitted.

Reviewed 1 of 1 files at r4.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @CydeWeys and @mindhog)


build.gradle, line 144 at r3 (raw file):

Previously, mindhog (Michael Muller) wrote…

Not quite:
gcpProject == null means that either the environment is not legal for deployment OR that the environment was not explicitly specified. I think we should explicitly require specifying an environment for "deploy", do you agree?

I guess. I'm fine with staging and deploying to alpha without any command line arguments though (therefore changing the previous if block). If you think deployment should always have an explicit flag, I'm OK with that.


build.gradle, line 157 at r4 (raw file):

crash,alpha

Can you change this to alpha, crash?

Copy link
Member

left a comment

OK it looks like we already explicitly specify the environment when building a RC. I think we are all good.

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @CydeWeys and @mindhog)

Copy link
Member Author

left a comment

Reviewable status: 3 of 4 files reviewed, 4 unresolved discussions (waiting on @CydeWeys and @jianglai)


build.gradle, line 144 at r3 (raw file):

Previously, jianglai (Lai Jiang) wrote…

I guess. I'm fine with staging and deploying to alpha without any command line arguments though (therefore changing the previous if block). If you think deployment should always have an explicit flag, I'm OK with that.

Done.


build.gradle, line 157 at r4 (raw file):

Previously, jianglai (Lai Jiang) wrote…
crash,alpha

Can you change this to alpha, crash?

Done.

Copy link
Member Author

left a comment

Reviewable status: 3 of 4 files reviewed, 4 unresolved discussions (waiting on @CydeWeys and @jianglai)


build.gradle, line 122 at r1 (raw file):

Previously, mindhog (Michael Muller) wrote…

Yep. Better to suffer shame in the review than to push to prod while testing the code to prevent pushing to prod :-) Fixed.

Done.


build.gradle, line 127 at r1 (raw file):

Previously, mindhog (Michael Muller) wrote…

Added a TODO.

Done.

Copy link
Member Author

left a comment

Reviewable status: 3 of 4 files reviewed, 4 unresolved discussions (waiting on @CydeWeys and @jianglai)


build.gradle, line 122 at r1 (raw file):

Previously, mindhog (Michael Muller) wrote…

Done.

@CydeWeys - sorry, missed that there was another question there. Open source users will be ok to build but they'll have to change this map locally to deploy (just as they need to now).

Copy link
Member

left a comment

:lgtm:

Reviewable status: 3 of 4 files reviewed, 2 unresolved discussions (waiting on @jianglai)

@mindhog mindhog merged commit 31b5abb into google:master Oct 4, 2019
4 of 7 checks passed
4 of 7 checks passed
code-review/reviewable 1 file, 2 discussions left (jianglai)
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
@mindhog mindhog deleted the mindhog:no-prod-deploy2 branch Oct 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.