Credential enhancements#9
Conversation
The default was clashing with another tool's credentials. Adds a new default env token COSTPULLER_CREDENTIALS which will take precedence over GOOGLE_APPLICATION_CREDENTIALS, if present. Enhances logging around credential discovery and usage, and moves those under `-debug` output when needed. This helped me diagnose the clash. Assisted by: Cursor
|
The PR looks good — the COSTPULLER_CREDENTIALS env var is a clean solution, the debug logging is well-gated, and replacing log.Fatalf with error returns in getCachedTokenSafe is a good improvement. One small suggestion: line 203 in gcloud_oauth.go — consider keeping "The token will not be cached." as log.Println rather than oauthDebugf, since that indicates a config issue users would want to see without needing -debug. Everything else looks good to merge. /Ciara |
Reverting the change on this line so that it will display without needing -debug. In the original PR, it was shifted from normal output to debug only.
|
Thank you for the review, @ciaramulligan, it auto-dismissed your suggestion as stale when I pushed a change to revert that line and I went ahead and resolved the thread. If anything else comes to mind, let me know. |
There was a problem hiding this comment.
There's a call to log.Println() that I think you'll want to correct. Otherwise, I just have some comments for your consideration.
Rename creds env, central -debug, unified getToken fall-through, and clearer OAuth failure context without duplicating ADC discovery (removes logCredentialSearchLocations).
|
Thanks for the review @webbnh, I believe this new commit addresses all your feedback. Here's the summary:
Let me know if you have additional suggestions or concerns. |
webbnh
left a comment
There was a problem hiding this comment.
Looks good; just two nits.
| // new token. When the cache path cannot be resolved, the cache file is missing, | ||
| // or the cached token cannot be refreshed, execution falls through to a single | ||
| // getNewToken path at the end (same as the original control flow). |
There was a problem hiding this comment.
Comments like "same as the original control flow" are not useful: they will be read in the future, when memories of "the original flow" are gone (and, the fact that the flow is the same as it used to be won't be useful information).
In a similar vein, comments which describe obvious aspects of the code don't add any value above and beyond what the code already provides. So, I would omit the whole last sentence; however, if you really want to keep it, I would have it indicate that, in those three cases, a new token is produced (which is what getNewToken() does, but naming the function here isn't as helpful as describing why we're calling it).
| if removeErr := os.Remove(tokenCachePath); removeErr != nil { | ||
| log.Printf("[getToken] Warning: unable to delete invalid cached token: %v", removeErr) | ||
| } |
There was a problem hiding this comment.
I still don't see any benefit to attempting to remove this file here.
The default was clashing with another tool's credentials. Adds a new default env token COSTPULLER_CREDENTIALS which will take precedence over GOOGLE_APPLICATION_CREDENTIALS, if present. Enhances logging around credential discovery and usage, and moves those under
-debugoutput when needed. This helped me diagnose the clash.Assisted by: Cursor