Skip to content

Quota project fix#8987

Closed
aabmass wants to merge 3 commits into
GoogleCloudPlatform:mainfrom
aabmass:quota-project-fix
Closed

Quota project fix#8987
aabmass wants to merge 3 commits into
GoogleCloudPlatform:mainfrom
aabmass:quota-project-fix

Conversation

@aabmass

@aabmass aabmass commented Jan 26, 2024

Copy link
Copy Markdown
Contributor

Description

Cloud Trace API requires a quota project. This change passes through GOOGLE_CLOUD_QUOTA_PROJECT environment variable, which Cloud Shell has automatically set. This is better than doing the resource project override and works better off Cloud Shell. I also renamed the file to reflect that it works with any credentials file, including SA keys.

Equivalent of GoogleCloudPlatform/golang-samples#3760 from the Go repo

Checklist

  • I have followed Sample Format Guide
  • pom.xml parent set to latest shared-configuration
  • Appropriate changes to README are included in PR
  • These samples need a new API enabled in testing projects to pass (let us know which ones)
  • These samples need a new/updated env vars in testing projects set to pass (let us know which ones)
  • Tests pass: mvn clean verify required
  • Lint passes: mvn -P lint checkstyle:check required
  • Static Analysis: mvn -P lint clean compile pmd:cpd-check spotbugs:check advisory only
  • This sample adds a new sample directory, and I updated the CODEOWNERS file with the codeowners for this sample
  • This sample adds a new Product API, and I updated the Blunderbuss issue/PR auto-assigner with the codeowners for this sample
  • Please merge this PR for me once it is approved

@product-auto-label product-auto-label Bot added the samples Issues that are directly related to samples. label Jan 26, 2024

@minherz minherz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is contra

Comment on lines -29 to +31
user: ${USERID}
user: $USERID

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is a reason for this change? To be syntactically correct the original line should be enclosed with quotes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change is to be consistent with L35. Either style should be equivalent.

To be syntactically correct the original line should be enclosed with quotes.

I'm not sure what you mean, what is wrong with this syntax?

Comment on lines -19 to +20
# export GOOGLE_APPLICATION_CREDENTIALS=$HOME/.config/gcloud/application_default_credentials.json
# docker compose -f docker-compose.yaml -f docker-compose.adc.yaml up
# GOOGLE_CLOUD_PROJECT="otel-quickstart-demos" \
# GOOGLE_APPLICATION_CREDENTIALS="$HOME/.config/gcloud/application_default_credentials.json" \

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is a reason to advertise the anti-pattern of using the GSA key file for authentication?
the doc samples use the main use case which is to run the sample on Google Cloud.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is to allow users to run the example on their own machine. We are not recommending this pattern in the docs, just making it possible.

user: $USERID
volumes:
- ${GOOGLE_APPLICATION_CREDENTIALS:-/dev/null}:/tmp/keys/gcp-credentials.json:ro
- ${GOOGLE_APPLICATION_CREDENTIALS?}:${GOOGLE_APPLICATION_CREDENTIALS}:ro

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if I am not mistake this syntax results in failing if the GOOGLE_APPLICATION_CREDENTIALS env var is not set.
what is a reason for using this construct in the code sample? see my previous comment about using anti-patterns

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct that is the purpose, to alert the user they need the environment variable with this docker compose file. If they want to get credentials from the environment (which doesn't work on a local machine), they can use the other compose file. The two options are explained in the README and in the devsite docs.

@aabmass aabmass requested a review from minherz January 29, 2024 16:36
@aabmass

aabmass commented Jan 31, 2024

Copy link
Copy Markdown
Contributor Author

Closing in favor of moving this into opentelemetry-operations-java.

@aabmass aabmass closed this Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

samples Issues that are directly related to samples.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants