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: default to XDG spec for global config #1530

Closed
wants to merge 1 commit into from
Closed

feat: default to XDG spec for global config #1530

wants to merge 1 commit into from

Conversation

evans
Copy link
Contributor

@evans evans commented Nov 6, 2020

- Summary

Thank you for making tools with slick dx(love the branch previews)! I was looking into #526 as a first issue 🎉 , which requests the XDG specification for the global netlify configuration. In other words use to ~/.config/netlify ... instead of ~/.netlify/config.json.

This PR suggests using ~/.netlify/config.json if it exists. Otherwise configure configstore to place the config in ~/.config/netlify/config.json or $XDG_CONFIG_HOME/netlify/config.json. I'm not fully happy with this plan, so I'm open to suggestions.

If config.json is retrieved in more places than the netlify cli(source), then I'm happy to add that to this PR if possible! Also if the ~/.netlify folder is used for more files, I recommend closing this PR and discussing the feasibility on #526 . I also worry about documentation, testing, and code style. Please let me know if there's something I missed.

Closes #526

My understanding of netlify configurations(not necessary to look at) To frame the problem, there are three different configurations that I've come across: `netlify.toml`, `.netlify/config.json`, and `.netlify/state.json`. As I understand, their functions are:
  • netlify.toml: build settings(ex: use npm run vs jekyll build), deploy settings, and env variables
  • ~/.netlify/config.json: global netlify state, such as userId and auth tokens
  • .netlify/state.json: local netlify state, such as siteId

Their path resolution:

- Test plan

I can add a test that uses mock-fs, though we may run into issues with mock-fs not playing nice with tests using node's fs (tschaub/mock-fs#103) and I vaguely remember having difficulty across os's.

test.afterEach(async (t) => {
await fs.rmdirRecursiveAsync(t.context.binPath)
})

I've run locally, though I don't consider that worth much for maintainability, so I'd love suggestions here!

- Description for the changelog

feat: default to XDG spec for global config when ~/.netlify/config.json doesn't exist.

- A picture of a cute animal (not mandatory but encouraged)

༼ つ ◕_◕ ༽つ

when ~/.netlify/config.json is not present use
$XDG_CONFIG_HOME/netlify/config.json, which falls back to
~/.config/netlify/config.json if $XDG_CONFIG_HOME is not set
@evans evans requested a review from a team as a code owner November 6, 2020 09:01
const globalConfigDefaults = {
/* disable stats from being sent to Netlify */
telemetryDisabled: false,
/* cliId */
cliId: uuidv4(),
}

const legacyConfigPath = getPathInHome(['config.json'])
Copy link
Contributor Author

@evans evans Nov 6, 2020

Choose a reason for hiding this comment

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

We could look for the folder instead in case there are other files in ~/.netlify

@erezrokah erezrokah added the type: feature code contributing to the implementation of a feature and/or user facing functionality label Nov 8, 2020
@erezrokah erezrokah self-requested a review November 10, 2020 16:51
@ehmicky
Copy link
Contributor

ehmicky commented Dec 3, 2020

Hi @evans,

Thanks a lot for this feature!
XDG does sound like a good idea.

However, this not apply to Windows users. For them, I believe using a library like env-paths would be more proper. This uses XDG for Linux but use more OS-idiomatic locations for macOS and Windows. On a typical machine this would be:

  • /home/evans/.config/netlify/config.json for Linux
  • /Users/evans/Library/netlify/config.json for macOS
  • C:\Users\evans\AppData\Roaming\netlify\Config\config.json for Windows

This can be easily handled with the configPath option of configstore. What do you think?

@erezrokah
Copy link
Contributor

To add to @ehmicky comment, would it make sense to change the file exports to a function?
By doing that we could:

  1. Have better error handling.
  2. Avoid calling fs.existsSync (which can fail) when the file is required.
  3. Make it easier to have backward compatibility - for example we could read the old config, provide it as defaults and then remove the old config file. With this approach we can be sure the config file is always located in a single place.

@evans
Copy link
Contributor Author

evans commented Jan 6, 2021

@ehmicky @erezrokah I have some embarrassing news. I've started a new job and won't have the time to devote to open source outside of that scope, so I won't be pushing this PR forward.

In terms of your comments, I agree with the decision to make the retrieval of the config a function and env-path sounds like a great move too! As an macOS user, I love the thought of putting it inside Library 🎉 I imagine some warning or messaging would be useful to the user in case they're expecting the config to stay put, for example if they symlink their configuration from somewhere else.

I wish that I could make the time/brainspace to make the changes. If you have bandwidth, feel free to take the branch over. If not, feel free to close the PR. In any case, thank you for your reviews and work on the Netlify CLI! I'm rooting for you and look forward to the developer wins you bring 💪

@erezrokah
Copy link
Contributor

I've started a new job and won't have the time to devote to open source outside of that scope, so I won't be pushing this PR forward.

Congrats on the new job and thank you for following up. Since the PR and the issue make sense we'll try to push it forward.

@ehmicky
Copy link
Contributor

ehmicky commented Jan 6, 2021

@evans Thanks for stirring us in the right direction with this, we'll take it from there. Congratulations on the new job!

@erezrokah
Copy link
Contributor

Closing this PR for #1725. Thanks again @evans for pushing this

@erezrokah erezrokah closed this Jan 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs docs type: feature code contributing to the implementation of a feature and/or user facing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Respect XDG Base Directory Specification
3 participants