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

Save config to standard directories #3

Closed
wants to merge 2 commits into from
Closed

Save config to standard directories #3

wants to merge 2 commits into from

Conversation

Siilwyn
Copy link
Contributor

@Siilwyn Siilwyn commented Aug 6, 2016

Instead of placing the configuration file in the home directory it should be placed in the appropriate directory per OS. This doesn't introduce any breaking changes since it falls back to using the configuration in the home directory.
Atom has the same issue open, I got most of the paths and how to to implement this gracefully from there.

@Siilwyn
Copy link
Contributor Author

Siilwyn commented Aug 6, 2016

Seems like the build fails because standard doesn't like the semicolons, if it's needed I can remove those but it feels very wrong to do that. I know I'm getting into a hot topic here but could you please read the "Error Correction" chapter at this page? And then tell me if you still want the semicolons removed.

@janl
Copy link

janl commented Aug 6, 2016

This is a good PR, thank you!

Let's not discuss coding style for the moment, could you remove the semicolons, so our linter is happy? Thanks! :)

@Siilwyn
Copy link
Contributor Author

Siilwyn commented Aug 6, 2016

Alright, sorry I couldn't keep myself from bringing it up. 😃
Should be good now.

@Siilwyn
Copy link
Contributor Author

Siilwyn commented Aug 12, 2016

@janl should be good now, right?

@janl
Copy link

janl commented Aug 12, 2016

cc @christophwitzko @finnp @boennemann as I’m on vacation 🌴

@Siilwyn
Copy link
Contributor Author

Siilwyn commented Aug 12, 2016

Thanks for letting me know, 🌞 enjoy your vacation @janl!

@boennemann
Copy link
Member

Hey @Siilwyn,

thanks for this improvement. I'm wondering whether we could encapsulate this code into it's own module. I'd like to avoid having this in here, and then it's also in Atom, and probably elsewhere.

What do you think about creating a separate module that we could depend on in here?

Best,
Stephan

@Siilwyn
Copy link
Contributor Author

Siilwyn commented Aug 17, 2016

Hi @boennemann,

that sounds like a great idea. I just did more research on existing modules and think it would be a good idea to create a module that returns the OS-specific paths for cache, config and app data. So without actually writing or reading, keeping the scope small enough but also big enough to be useful. What are your thoughts on this?
And how would you name it? I'm thinking of: os-store-paths, os-store-dirs or os-xdg-dirs.

Usage would look like this in the existing code:

var osStorePaths = require('os-store-paths')
// or var osConfigDir = require('os-store-paths').config()
var config

var getConfigDir = function () {
  // If configuration exists in the old default path use it
  try {
    fs.accessSync(path.join(home(), '.greenkeeperrc'))
    return home()
  } catch (err) {
    // Use OS-specific configuration directory
    return osStorePaths.config()
    // or return osConfigDir;
  }
}

var configPath = path.join(getConfigDir(), '.greenkeeperrc')

try {
  config = JSON.parse(fs.readFileSync(configPath))
} catch (e) {
  config = {}
}

@finnp
Copy link
Member

finnp commented Aug 22, 2016

Hey, this is a great PR ☺️

A module for this sounds great, but better would be using an existing one. I found https://github.com/sindresorhus/conf, which is based on https://github.com/sindresorhus/env-paths which could be useful here.

@Siilwyn
Copy link
Contributor Author

Siilwyn commented Aug 27, 2016

Thanks @finnp :)

Didn't know such a module already existed, just pushed a chance to use this module instead. I've kept the original commit because I think it adds to the progression but I can squash them too if you prefer.

@Siilwyn
Copy link
Contributor Author

Siilwyn commented Aug 31, 2016

Any update on this? I'm excited to see this getting merged. :)

@boennemann boennemann closed this in 2b5d79c Sep 2, 2016
boennemann pushed a commit that referenced this pull request Sep 2, 2016
The config is read to and written from standard config directories using the "env-paths" module,
rather than just stuffing it into the home directory.
Old config files will automatically be migrated to the new location.

BREAKING CHANGE: Config is no longer stored in `~/.greenkeeperrc`

Closes #3, Closes #5
@boennemann
Copy link
Member

Hey @Siilwyn,

thanks so much for your work here. I landed this as 8384cb1.

I also added a migration, so that when someone uses the old file it's automatically moved into the new location.

Thanks so much,
Stephan

@boennemann
Copy link
Member

I just updated the main CLI with this new version: greenkeeperio/greenkeeper#282

I added a new command, so that greenkeeper config path now shows where we're storing config.

@Siilwyn
Copy link
Contributor Author

Siilwyn commented Sep 2, 2016

Awesome! Thank you @boennemann :)

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.

4 participants