Load default credentials from right path on Windows #112
Conversation
On Windows, the default credentials file is located in the `%APPDATA%\gcloud\application_default_credentials.json`. The existing implementation was looking for credentials in `$HOME/.config/gcloud/application_default_credentials.json`, which only works on MacOS and Linux. This change creates a `get_user_credentials_path()` function that has different implementations for Linux/MacOS and Windows platforms.
src/config_default_credentials.rs
Outdated
| let mut home = home::home_dir().ok_or(Error::Str("home directory not found"))?; | ||
| home.push(USER_CREDENTIALS_PATH); | ||
| let home = get_user_credentials_path()?; | ||
| debug!("try to load credentials from {:?}", home); |
There was a problem hiding this comment.
Maybe it would be better to split this into two debug!() calls? I'd like to have some logging that announces trying the ConfigDefaultCredentials method that happens anyway, even if we fail to get the home directory (on Unix) or the APPDATA environment variable.
Also, bit of a pre-existing issue, we should probably use structured logging instead of formatting the directory into the log message (so use path = home before the message in the debug!() calls).
So maybe something like this?
debug!("try to load credentials from configuration");
let mut config_path = config_dir()?;
config_path.push(USER_CREDENTIALS_PATH);
debug!(config = config_path, "reading configuration file");There was a problem hiding this comment.
Added the two debug calls, also renamed home to config_path and extracted concatenating USER_CREDENTIALS_PATH to outside the config_dir function.
src/config_default_credentials.rs
Outdated
| @@ -103,5 +106,31 @@ struct RefreshRequest<'a> { | |||
| refresh_token: &'a str, | |||
| } | |||
|
|
|||
| #[cfg(any(target_os = "linux", target_os = "macos"))] | |||
| fn get_user_credentials_path() -> Result<PathBuf, Error> { | |||
There was a problem hiding this comment.
We should avoid get_() prefixes per the API naming guidelines. Also I think it would be nice to deduplicate the part where we push the USER_CREDENTIALS_PATH, so how about we name this config_dir()?
There was a problem hiding this comment.
Renamed to config_dir() and extracted the USER_CREDENTIALS_PATH bit.
src/config_default_credentials.rs
Outdated
| if !home.exists() { | ||
| return Err(Error::Str("APPDATA directory not found")); | ||
| } | ||
| home.push(USER_CREDENTIALS_PATH); | ||
| Ok(home) |
There was a problem hiding this comment.
In light of the above, would like to spell this as follows:
match home.exists() {
true => Ok(home)
false => Err(Error::Str("APPDATA directory not found")),
}
|
@djc Thank you for the review. Addressed the comments and re-tested on Windows / MacOS. PTAL. |
On Windows, the default credentials file is located in the
%APPDATA%\gcloud\application_default_credentials.json. The existing implementation was looking for credentials in$HOME/.config/gcloud/application_default_credentials.json, which only works on MacOS and Linux.This change creates a
config_dir()function that has different implementations for Linux/MacOS and Windows platforms, and returns the appropriate path for each.