-
Notifications
You must be signed in to change notification settings - Fork 321
Try to find the active gcloud config #348
Try to find the active gcloud config #348
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
70930c1
to
d17cce4
Compare
Hi Rafal, Thanks -- this looks like a nice improvement. It looks like it will apply equally well to Apache Beam (incubating), which will be the foundation for this SDK starting in version 2.0. We are try to add new features in Beam first, then backport them here -- this prevents future regressions. Would you mind starting this PR there, and then we can backport it here? The Beam Contribution Guide is the place to get started. Thanks! |
CLA confirmed for Spotify. |
// get active configuration | ||
File activeConfFile = new File( | ||
getUserHome(), | ||
".config/gcloud/active_config"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do the paths specified in these locations work on windows?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question - there is if
branch for windows
before, also previous version of the code assumed the same hierarchy for all platforms (here). What would you suggest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems as though Google is vending a library that already does this and instead of Dataflow owning this logic, we can rely on this library but wanted to make sure that this meets your needs:
https://github.com/GoogleCloudPlatform/gcloud-java/blob/master/gcloud-java-core/src/main/java/com/google/cloud/ServiceOptions.java
Look at the logic for defaultProject(), specifically activeGoogleCloudConfig() and googleCloudProjectId()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry @lukecwik - was on vacation. I see, thanks for the gcloud-java link, I looked at the code, and it's pretty much the same. afaiu current DF does not check for:
String projectId = System.getProperty(PROJECT_ENV_NAME, System.getenv(PROJECT_ENV_NAME));
if (projectId == null) {
projectId = appEngineProjectId();
}
if (projectId == null) {
projectId = serviceAccountProjectId();
}
Specifically serviceAccountProjectId
could be interesting, afaiu it would discover if user is using a service account through GOOGLE_APPLICATION_CREDENTIALS
and then if available, fetch the project from the credentials file. One could argue if that is good or bad idea, probably good for defaultProject
.
It seems like we could push this PR, I could submit PR to gcloud-java to abstract the project discover logic away from ServiceOptions
, then I could submit another PR to DF to start using gcloud-java? Does it sound like a good plan?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ping @lukecwik
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping 2x @lukecwik - do you think we should wait for google-cloud-java or move forward?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would wait for google-cloud-java, it seems as though they are working towards the change as evidenced on googleapis/google-cloud-java#1207
898e3d6
to
4261b7b
Compare
Opened up googleapis/google-cloud-java#1207 |
This is superseded by the follow feature request on Apache Beam 2.x: |
No description provided.