-
Notifications
You must be signed in to change notification settings - Fork 310
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
feat: add get_bq_config_path() to _cloud_sdk.py #1358
Conversation
I believe the kokoro failures are unrelated to my change, but this is my first time making a change to this repo so please advise. |
google/auth/_cloud_sdk.py
Outdated
@@ -42,8 +44,11 @@ | |||
) | |||
|
|||
|
|||
def get_config_path(): | |||
"""Returns the absolute path the the Cloud SDK's configuration directory. | |||
def get_config_path(config_directory=_CONFIG_DIRECTORY): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should wrap this function with a gcloud_get_config_path
so the intent is clearly documented.
Would it be a large change to modify existing uses of get_config_path
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated, not too much overhead to modify existing uses of get_config_path
@wj-chen I refreshed the test credentials. The next test run should have all the noise removed. |
Thank you! There appears to be lint errors. Running
|
The interpreter for the lint step uses Python 3.8. I suggest installing it via pyenv. Here is a sample that will give you a shell that has 3.8. $ pyenv install 3.8.15
$ pyenv shell 3.8.15
$ python --version # Confirm the interpreter is 3.8. Now the active shell should have access to 3.8. |
Thanks! Linter would run now but would result in an
|
Do you mind sharing the full trace with me either here or offline? I don't think I have enough info to help debug. |
Apologies that this has turned into a troubleshooting session but much appreciate your assistance! Sharing the stack trace here for visibility:
|
Ah interesting. I do not have this issue. Can you try working from your cloudtop's file system, instead of Piper? |
Thank you! I managed to resolve it. Turned out to be a faulty Linter failure has been fixed. default_explicit_authorized_user-3.7 and default_explicit_authorized_user_explicit_project-3.7 tests failed again though... |
Thanks for the credential refresh! The checks have passed. |
@@ -42,32 +44,53 @@ | |||
) | |||
|
|||
|
|||
def get_config_path(): | |||
"""Returns the absolute path the the Cloud SDK's configuration directory. | |||
def get_config_path(config_directory=_CONFIG_DIRECTORY_GCLOUD): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A cursory look suggests this is only used in gcloud's code base.
Can you own making sure that there is no regression when this library is updated in Piper?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes of course. I was actually making a change in the gcloud code base first and that's when I realized the path definitions happen here. How often do you merge into google3 and when will this change be expected to land?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The merge is manual. I can provide instructions for how you can do it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. Could you help me reach @sai-sunder-s for a review so I can proceed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @clundin25, I've received an approval from @sai-sunder-s and am ready for merge instructions now.
Hi @sai-sunder-s , let me know if you have any questions about this change |
)" This reverts commit 9f52f66.
Will be used by the Cloud CLI's config to write a credential file for the
bq
tool to use. Part of the work to support BQ CLI's auth library upgrade from oauth2client to google-auth.