fix: refactor error reporting code sample #3291
Conversation
product-auto-label
bot
added
the
samples
minherz
force-pushed
the
minherz/fix-275730159
branch
from
cf5ca01
to
e5a3667
Compare
| // Sets your Google Cloud Platform project ID. | ||
| projectID := "YOUR_PROJECT_ID" | ||
| // TODO: Sets your Google Cloud Platform project ID via environment or explicitly | ||
| projectID := os.Getenv("GOOGLE_PROJECT_ID") |
There was a problem hiding this comment.
Why add this? Also, if you are going to add it, GOOGLE_CLOUD_PROJECT is the standard environment variable, not GOOGLE_PROJECT_ID.
There was a problem hiding this comment.
We try to align samples across all languages to exhibit the same behavior. The options are:
- Environment variable
- Explicit setup
- Fallback to project ID implicitly configured at client initialization
Unfortunately I did not have time to dig for implementation of the last bullet in Golang. I will appreciate it if you can point me to the code that "retrieves" project ID from the client settings.
There was a problem hiding this comment.
+1 to changing to GOOGLE_CLOUD_PROJECT
| projectID := os.Getenv("GOOGLE_PROJECT_ID") | |
| projectID := os.Getenv("GOOGLE_CLOUD_PROJECT") |
... primarily because the rest of this repository uses GOOGLE_CLOUD_PROJECT (see search results).
There's no use of GOOGLE_PROJECT_ID in this repo.
There was a problem hiding this comment.
I am not sure autodetecting the project ID is supported in all clients for Go; see discussion on googleapis/google-cloud-go#1294 . @codyoss please correct me if I'm wrong.
In snippet samples we generally pass the project ID in via an arg (see https://googlecloudplatform.github.io/samples-style-guide/#minimal-arguments ). As this is a quickstart, I think it's fine to try to detect it from the environment. However I do think we should standardize on GOOGLE_CLOUD_PROJECT rather than GOOGLE_PROJECT_ID given that is what is supported across tools.
There was a problem hiding this comment.
Changes made to address this while allowing users to provide the project ID via argument.
There was a problem hiding this comment.
Thanks for working on these updates related to b/275730159!
Approved (since the remaining GOOGLE_CLOUD_PROJECT issue is a nit, IMO).
Looks like the Go / Lint check is passing.
It sounds like we just need to run:
goimports -w . && find . -name go.mod -execdir go mod tidy \;
84f5dbc
to
e5a3667
Compare
align code sample for error reporting with code in other languages. use an explicit error with the "fixed" message
fix typo in the comment. expand the comment about error context autopopulation. address lint warning for errors.New() call.
align the code to the way project ID is set in other languages. use "official" env variable name.
minherz
force-pushed
the
minherz/fix-275730159
branch
from
1b93005
to
a38f891
Compare
align code sample for error reporting with code in other languages (b/275730159). use an explicit error with the "fixed" message. populate the service context with the name of the service (module) and the version.