Well known file support for GoogleCredential.getDefault.

Well known file support for GoogleCredential.getDefault.

[Orest, 2014-04-23 23:53:58]

LGTM

https://codereview.appspot.com/92730043/diff/1/google-api-client/src/main/jav...
File
google-api-client/src/main/java/com/google/api/client/googleapis/auth/oauth2/DefaultCredentialProvider.java
(right):

https://codereview.appspot.com/92730043/diff/1/google-api-client/src/main/jav...
google-api-client/src/main/java/com/google/api/client/googleapis/auth/oauth2/DefaultCredentialProvider.java:142:
File configPath = new java.io.File(getProperty("user.home", ""), ".config");
Any particular reason to fully qualify the File class?

https://codereview.appspot.com/92730043/diff/1/google-api-client/src/test/jav...
File
google-api-client/src/test/java/com/google/api/client/googleapis/auth/oauth2/DefaultCredentialProviderTest.java
(right):

https://codereview.appspot.com/92730043/diff/1/google-api-client/src/test/jav...
google-api-client/src/test/java/com/google/api/client/googleapis/auth/oauth2/DefaultCredentialProviderTest.java:386:
}
These two tests are almost identical.
Would it be worth factoring the common part into a function (with plenty of
arguments) so that we don't repeat the code?

[Anthony Moore, 2014-04-30 18:17:41]

Addressed all comments.

https://codereview.appspot.com/92730043/diff/1/google-api-client/src/main/jav...
File
google-api-client/src/main/java/com/google/api/client/googleapis/auth/oauth2/DefaultCredentialProvider.java
(right):

https://codereview.appspot.com/92730043/diff/1/google-api-client/src/main/jav...
google-api-client/src/main/java/com/google/api/client/googleapis/auth/oauth2/DefaultCredentialProvider.java:142:
File configPath = new java.io.File(getProperty("user.home", ""), ".config");
On 2014/04/23 23:53:59, Orest wrote:
> Any particular reason to fully qualify the File class?
No. Fixed.

https://codereview.appspot.com/92730043/diff/1/google-api-client/src/test/jav...
File
google-api-client/src/test/java/com/google/api/client/googleapis/auth/oauth2/DefaultCredentialProviderTest.java
(right):

https://codereview.appspot.com/92730043/diff/1/google-api-client/src/test/jav...
google-api-client/src/test/java/com/google/api/client/googleapis/auth/oauth2/DefaultCredentialProviderTest.java:386:
}
On 2014/04/23 23:53:59, Orest wrote:
> These two tests are almost identical.
> Would it be worth factoring the common part into a function (with plenty of
> arguments) so that we don't repeat the code?

Done.

[Anthony Moore, 2014-04-30 18:40:24]

Fixed a permission bug when running on App Engine.

[Orest, 2014-04-30 21:09:23]

LGTM

[beaton, 2014-05-06 20:34:12]

https://codereview.appspot.com/92730043/diff/60001/google-api-client/src/main...
File
google-api-client/src/main/java/com/google/api/client/googleapis/auth/oauth2/DefaultCredentialProvider.java
(right):

https://codereview.appspot.com/92730043/diff/60001/google-api-client/src/main...
google-api-client/src/main/java/com/google/api/client/googleapis/auth/oauth2/DefaultCredentialProvider.java:48:
static final String CLOUDSDK_CONFIG_WORD = "gcloud";
how about CLOUDSDK_CONFIG_DIRECTORY?

https://codereview.appspot.com/92730043/diff/60001/google-api-client/src/main...
google-api-client/src/main/java/com/google/api/client/googleapis/auth/oauth2/DefaultCredentialProvider.java:136:
InputStream credentialsStream = new FileInputStream(wellKnownFileLocation);
I think you need a finally block to close this stream.

https://codereview.appspot.com/92730043/diff/60001/google-api-client/src/main...
google-api-client/src/main/java/com/google/api/client/googleapis/auth/oauth2/DefaultCredentialProvider.java:163:
if (os.indexOf("win") >= 0) {
this check seems brittle, I can easily imagine somebody shipping "Full of Win
Linux" or something like that.

Can we do something less likely to result in a false positive?

[Anthony Moore, 2014-05-12 20:08:39]

Fixed all open comments.

https://codereview.appspot.com/92730043/diff/60001/google-api-client/src/main...
File
google-api-client/src/main/java/com/google/api/client/googleapis/auth/oauth2/DefaultCredentialProvider.java
(right):

https://codereview.appspot.com/92730043/diff/60001/google-api-client/src/main...
google-api-client/src/main/java/com/google/api/client/googleapis/auth/oauth2/DefaultCredentialProvider.java:48:
static final String CLOUDSDK_CONFIG_WORD = "gcloud";
On 2014/05/06 20:34:12, beaton wrote:
> how about CLOUDSDK_CONFIG_DIRECTORY?

Done.

https://codereview.appspot.com/92730043/diff/60001/google-api-client/src/main...
google-api-client/src/main/java/com/google/api/client/googleapis/auth/oauth2/DefaultCredentialProvider.java:136:
InputStream credentialsStream = new FileInputStream(wellKnownFileLocation);
On 2014/05/06 20:34:12, beaton wrote:
> I think you need a finally block to close this stream.

Done.

https://codereview.appspot.com/92730043/diff/60001/google-api-client/src/main...
google-api-client/src/main/java/com/google/api/client/googleapis/auth/oauth2/DefaultCredentialProvider.java:163:
if (os.indexOf("win") >= 0) {
On 2014/05/06 20:34:12, beaton wrote:
> this check seems brittle, I can easily imagine somebody shipping "Full of Win
> Linux" or something like that.
> 
> Can we do something less likely to result in a false positive?
Started email thread.
Changing to windows.

[beaton, 2014-05-12 20:10:26]

LGTM