Fix ADC detection for Google Cloud Shell - #60
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). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
Hi @pran-ram , thanks for the PR - could you explain the error you've seen before and the improvement with the code changes provided with the ADC credentials? |
|
@p-habib-eshwar - here's a description of this proposed change: In Google Cloud Shell (and some containerized/CI-CD environments), credentials are provisioned dynamically via environment variables or an internal metadata server rather than a physical file stored at $HOME/.config/gcloud/application_default_credentials.json. Under the old logic, running this script inside Google Cloud Shell would always trigger NEED_LOGIN=true (and issue a warning) because the physical file was missing, even though the environment already had fully authorized, valid credentials. The new logic recognizes the Cloud Shell credentials because gcloud auth application-default print-access-token succeeds in that environment. Instead of verifying how the credentials are stored (i.e., looking for a file at a hardcoded path), the script now simply tests if the credentials work. This makes the deployment script more resilient across different operating systems, custom configurations, and automated environments. The nested conditional statements have been flattened, reducing the logic from 12 lines of code down to 6 lines, making the deployment script easier to maintain. |
|
Thanks for expanding on the rationale. My primary motivation for the change was that the deployment script was treating the absence of $HOME/.config/gcloud/application_default_credentials.json as equivalent to “not authenticated”. In Google Cloud Shell, ADC is already available even though that file doesn’t exist, so the script incorrectly set NEED_LOGIN=true. The new approach validates that ADC is usable instead of assuming a specific credential storage mechanism, which also makes the check work across a wider range of environments. |
|
Hi @pran-ram, thanks for the contribution. The ADC change looks right, but this PR needs a few things before we can proceed as per docs/contributing.md:
Please update the PR accordingly and we can continue the review. |
No description provided.