fix regression on appEngine detection #1109
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@mattwhisenhunt PTAL Your commit 75ba03c causes a regression in some tests we don't have here. I'd like to either fully rollback that change (close out this PR, start a new PR) or go ahead with having both logic. Which do you think is better? And could you explain 75ba03c? I don't have the context for that unfortunately. |
|
I didn't know at the time there was another source of truth of this repo so I didn't squash my CL. This started out as a bit of a vague issue that Les and I used a couple of PRs as a back and forth to come to the result we wanted. |
mattwhisenhunt
approved these changes
Apr 25, 2018
| return false; | ||
| Class<?> systemPropertyClass = null; | ||
| try { | ||
| systemPropertyClass = forName("com.google.appengine.api.utils.SystemProperty"); |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
There are 3 cases: Java 7 on std, Java 8 on std, and Java compat . The change you are bringing back will look to see if the GAE libraries have been included. That won't always be the case. It's better to use the environment variables as that will tell you reality, that always work for local development. Typically, your trying to find out if you need to use the metadata server, or he GAE api's to get a credential. (Java 7) (ie. what is the issue you are really trying to fix) |
|
@lesv Thanks for the context. In that case, I'd like to go forward with this, since there are internal users that depend on the old case and it sounds like we're trying to convince external users to use the new environment variable case. Does that sound appropriate to you? |
|
@jadekler, go ahead and start an internal email thread and share the log results from the internal failing test. The test may be incorrect and should maybe be updated instead. |
|
Note - I should have mentioned above that there are 3 GAE cases, there are of course, other cases. On Java 8 - Credentials should be gotten from the Metadata server: Flexible runtime - use Metadata server Locally - there should not be these environment variables set, you should get them from the normal DefaultAuth or ADC algorithm. All other GCP runtimes should use the ADC algorithm that checks for |
|
It's not clear to me what case is being solved by this change. @lesv, you said "credentials should be gotten through the code you are adding". But the adds are only in the detection part, not in actually retrieving the credentials. Is this fixing Flex? Although it sounds like Flex should enable the |
lesv
suggested changes
Apr 26, 2018
| return false; | ||
| Class<?> systemPropertyClass = null; | ||
| try { | ||
| systemPropertyClass = forName("com.google.appengine.api.utils.SystemProperty"); |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| @@ -256,7 +258,6 @@ private GoogleCredential getCredentialUsingWellKnownFile( | |||
| } | |||
| } | |||
|
|
|||
|
|
|||
| private boolean runningOnAppEngine() { | |||
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| @@ -256,7 +258,6 @@ private GoogleCredential getCredentialUsingWellKnownFile( | |||
| } | |||
| } | |||
|
|
|||
|
|
|||
| private boolean runningOnAppEngine() { | |||
| if (getEnvEquals("GAE_ENV", "standard")) { | |||
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| @@ -271,7 +272,38 @@ private boolean runningOnAppEngine() { | |||
| return true; | |||
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
jeanbza
commented
Apr 27, 2018
| Object environmentValueValue = valueMethod.invoke(environmentValue); | ||
| return (environmentValueValue != null); | ||
| } catch (NoSuchFieldException exception) { | ||
| cause = exception; |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
@lesv PTAL - incorporated your fixes |
lesv
suggested changes
May 1, 2018
| @@ -256,7 +258,6 @@ private GoogleCredential getCredentialUsingWellKnownFile( | |||
| } | |||
| } | |||
|
|
|||
|
|
|||
| private boolean runningOnAppEngine() { | |||
| if (getEnvEquals("GAE_ENV", "standard")) { | |||
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| @@ -256,7 +258,6 @@ private GoogleCredential getCredentialUsingWellKnownFile( | |||
| } | |||
| } | |||
|
|
|||
|
|
|||
| private boolean runningOnAppEngine() { | |||
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| Object environmentValueValue = valueMethod.invoke(environmentValue); | ||
| return (environmentValueValue != null); | ||
| } catch (NoSuchFieldException exception) { | ||
| cause = exception; |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| @@ -271,7 +272,38 @@ private boolean runningOnAppEngine() { | |||
| return true; | |||
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
@lesv PTAL |
lesv
reviewed
May 1, 2018
| private boolean runningOnAppEngine() { | ||
| if (getEnvEquals("GAE_ENV", "standard")) { | ||
| private boolean useGAEStandardAPI() { | ||
| if (getEnvEquals("GAE_ENV", "standard") && getEnvEquals("GAE_RUNTIME", "java7")) { | ||
| return true; |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| return (environmentValueValue != null); | ||
| } catch (NoSuchFieldException ignored) { | ||
| // If the field does not exist then we treat it as false. | ||
| } catch (SecurityException exception) { |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| return true; | ||
|
|
||
| if (cause != null) { | ||
| throw OAuth2Utils.exceptionWithCause(new RuntimeException(String.format( |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
|
||
| if (cause != null) { | ||
| throw OAuth2Utils.exceptionWithCause(new RuntimeException(String.format( | ||
| "Unexpcted error trying to determine if runnning on Google App Engine: %s", |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| if (getEnvEquals("GAE_RUNTIME", "java8")) { | ||
| return true; | ||
| Exception cause = null; | ||
| Field environmentField; |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
LGTM |
|
You might wish to look at the tests. |
lesv
suggested changes
May 1, 2018
|
@lesv PTAL |
mattwhisenhunt
approved these changes
May 1, 2018
|
And the tests pass. 😁 |
|
Ahh ok, thanks @mattwhisenhunt that's good context 👍 |
lesv
approved these changes
May 1, 2018
|
Will merge this tomorrow if tap train gives thumbs up. |
jeanbza
added a commit
to jeanbza/google-api-java-client
that referenced
this pull request
Jul 5, 2018
The method runningOnAppEngine was recently renamed to useGAEStandardAPI, and its logic was changed in googleapis#1109 and googleapis@75ba03c. This commit reverts back to the original logic before either of those commits. The reason for the revert is that several deployments failed with this change internally. See b/111173267, b/111149869.
jeanbza
added a commit
that referenced
this pull request
Jul 9, 2018
The method runningOnAppEngine was recently renamed to useGAEStandardAPI, and its logic was changed in #1109 and 75ba03c. This commit reverts back to the original logic before either of those commits. The reason for the revert is that several deployments failed with this change internally. See b/111173267, b/111149869.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.