Skip to content

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

Merged
elramen merged 5 commits intomasterfrom
xdg_config_home
May 27, 2024
Merged

feat(auth): Allow global config to be located in XDG directory#2059
elramen merged 5 commits intomasterfrom
xdg_config_home

Conversation

@elramen
Copy link
Copy Markdown
Member

@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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member 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
.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
Copy Markdown
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
Copy Markdown
Member 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
Copy Markdown
Member 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 requested a review from szokeasaurusrex May 6, 2024 12:16
@elramen elramen self-assigned this May 14, 2024
@elramen elramen requested a review from sl0thentr0py May 17, 2024 10:06
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