Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: do not suppress external project ID determination errors #1153

Merged
merged 2 commits into from Apr 6, 2021
Merged

fix: do not suppress external project ID determination errors #1153

merged 2 commits into from Apr 6, 2021

Conversation

bojeil-google
Copy link
Contributor

Do not suppress the underlying error, as the error could contain helpful
information for debugging and fixing. This is especially true for
external account creds as in order to get the project ID, the following
operations have to succeed:

  1. Valid credentials file should be supplied.
  2. Ability to retrieve access tokens from STS token exchange API.
  3. Ability to exchange for service account impersonated credentials (if
    enabled).
  4. Ability to get project info using the access token from step 2 or 3.

Without surfacing the error, it is harder for developers to determine
which step went wrong.

Do not suppress the underlying error, as the error could contain helpful
information for debugging and fixing. This is especially true for
external account creds as in order to get the project ID, the following
operations have to succeed:
1. Valid credentials file should be supplied.
2. Ability to retrieve access tokens from STS token exchange API.
3. Ability to exchange for service account impersonated credentials (if
   enabled).
4. Ability to get project info using the access token from step 2 or 3.

Without surfacing the error, it is harder for developers to determine
which step went wrong.
@bojeil-google bojeil-google requested a review from a team as a code owner April 2, 2021 05:46
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Apr 2, 2021
@codecov
Copy link

codecov bot commented Apr 2, 2021

Codecov Report

Merging #1153 (7f92071) into master (ec49fe6) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1153   +/-   ##
=======================================
  Coverage   93.75%   93.76%           
=======================================
  Files          28       28           
  Lines        6260     6267    +7     
  Branches      656      611   -45     
=======================================
+ Hits         5869     5876    +7     
  Misses        391      391           
Impacted Files Coverage Δ
src/auth/googleauth.ts 97.63% <100.00%> (+0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ec49fe6...7f92071. Read the comment docs.

@bojeil-google
Copy link
Contributor Author

@bcoe I believe this should be a safe change to make. This is an unrecoverable error that is typically thrown during development and setup. It is now returning a clearer error message. An internal team member was having issues with it and surfaced it. Let me know if you have any questions.

Copy link
Contributor

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

I'm comfortable with this change 👍

@bcoe bcoe added the automerge Merge the pull request once unit tests and other checks pass. label Apr 6, 2021
@gcf-merge-on-green gcf-merge-on-green bot merged commit 6c1c91d into googleapis:master Apr 6, 2021
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Apr 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants