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

feat(auth): Allow global config to be located in XDG directory #2059

Merged
merged 5 commits into from
May 27, 2024

Conversation

elramen
Copy link
Contributor

@elramen elramen commented May 2, 2024

If the global config file does not exist in the home directory, look in the following paths:

  • Linux: $XDG_CONFIG_HOME/sentry/sentrycli.ini and $HOME/.config/sentry/sentrycli.ini
  • Mac: $HOME/Library/Application Support/sentry/sentrycli.ini
  • Windows: {FOLDERID_RoamingAppData}

This enables users to keep their home dir clean, adhering to standards such as XDG on linux.

Fixes GH-1521

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
Copy link
Member

@szokeasaurusrex szokeasaurusrex left a 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))
Copy link
Member

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?

Copy link
Contributor Author

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 Show resolved Hide resolved
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!")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

@elramen elramen May 3, 2024

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:

  1. Home dir if file exists there.
  2. Config dir if file exists there.
  3. Home dir no matter if the file exists there (just like before).
  4. Error if we weren't even able to read home dir (like before).

@elramen elramen self-assigned this May 14, 2024
@elramen elramen requested a review from sl0thentr0py May 17, 2024 10:06
@elramen elramen merged commit c987094 into master May 27, 2024
12 checks passed
@elramen elramen deleted the xdg_config_home branch May 27, 2024 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support $XDG_CONFIG_HOME for config file
3 participants