-
-
Notifications
You must be signed in to change notification settings - Fork 222
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(auth): Allow global config to be located in XDG directory #2059
Conversation
If .sentryclirc does not exist, look for the global config file in $XDG_CONFIG_HOME/sentry/sentrycli.ini and $HOME/.config/sentry/sentrycli.ini, allowing users to adhere to the XDG specifications. Fixes GH-1521
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.
Have some questions and a suggestion
src/config.rs
Outdated
.map(|mut path| { | ||
path.push(CONFIG_RC_FILE_NAME); | ||
path | ||
.map(|p| p.join(CONFIG_RC_FILE_NAME)) |
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.
Why are we changing from the previous path.push
to p.join
? And, is there a reason we need to make this change in this PR?
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.
Because it looks cleaner to havep.join(...)
instead of p.push(...); p
. push() doesn't return anything. The change is so small it would seem strange to have a separate PR for it.
src/config.rs
Outdated
.map(|p| p.join("sentry/sentrycli.ini")) | ||
.filter(|p| p.exists())) | ||
.ok_or_else(|| { | ||
format_err!("Could not find config file. Please run `sentry-cli login` and try again!") |
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.
format_err!("Could not find config file. Please run `sentry-cli login` and try again!") | |
format_err!("Could not find config file.") |
Why add the note about running sentry-cli login
here? I am unsure whether this is always necessary – auth token is not needed for all CLI requests. Even if we should be adding this, this change should be made in a separate PR, since it is unrelated to what we want to do in this change.
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.
You are right, we should default to the home dir path (like before) even if the file doesn't exist.
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 prioritization is now as follows:
- Home dir if file exists there.
- Config dir if file exists there.
- Home dir no matter if the file exists there (just like before).
- Error if we weren't even able to read home dir (like before).
If the global config file does not exist in the home directory, look in the following paths:
This enables users to keep their home dir clean, adhering to standards such as XDG on linux.
Fixes GH-1521