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

Get config defaults from config file #72

Closed
marten-de-vries opened this issue Sep 13, 2014 · 12 comments
Closed

Get config defaults from config file #72

marten-de-vries opened this issue Sep 13, 2014 · 12 comments

Comments

@marten-de-vries
Copy link
Member

Since commit pouchdb/express-pouchdb#103, app.couchConfig exists and is set by express-pouchdb. Now some command line options match 'couchdb' settings. How about using the config file as defaults for the command line options? This allows a second convenient and persistent way of setting them. Candidates would be:

[cors]
methods = ...
credentials = ...
headers = ...
origins is probably also specifyable in 'corser'?

[httpd]
port = ...
bind_address? (would be new, but nice)

[couchdb]
database_dir = ...

@nolanlawson
Copy link
Member

Hm, CouchDB's config files are written in Erlang, but I feel like ours should be written in JSON. This would also give us a nice 1-to-1 correspondence with what's shown in the actual _config REST API.

So yeah, I'm definitely +1 on this; it's a little silly for the settings not to persist after the server is shut down. I just want to make sure we figure out a good format first, since ideally people should feel comfortable modifying the file directly.

@marten-de-vries
Copy link
Member Author

The current _config implementation in express-pouchdb saves its data as a pretty printed config.json file in the current working directory. I tried using a config parser library to emulate .ini style config files like in CouchDB, but couldn't get that to work reliably, and JSON seems fine.

Example (you should be able to get something similar using the current express-pouchdb master branch):

marten@marten-laptop:~/git/pouchdb-server/bin$ cat config.json 
{
  "replicator": {
    "db": "_replicator"
  },
  "admins": {
    "marten": "-pbkdf2-4d3e92819df8ae536bc013c0348a1857b6a71951,14c18e15710717f1561431ff1b01e910a1f41d41ed1cf178,10"
  }
}marten@marten-laptop:~/git/pouchdb-server/bin$ 

(The password is just 'test', nothing sensitive...)

@nolanlawson
Copy link
Member

Ah, in that case, that is amazing. If you could document that in either the express-pouchdb or pouchdb-server, that would be really helpful. This config.json file seems like a much better option than the command-line arguments. Or at the very least, the configurations such as --in-memory could be exposed via _config.

@marten-de-vries
Copy link
Member Author

I think the best use case would be as a fallback for the command-line arguments. That way it's backwards compatible, easy to make a small change, but also possible to just ignore the command line args and use /_config. Documenting it in express-pouchdb is a good idea (it's defined there). Maybe some 'user' documentation in pouchdb-server, although since you can just use Fauxton to edit config like in CouchDB, that might not be necessary. I personally prefer that over config.json anyway.

As for options like --in-memory, while I'd like to make them configurable via _config, I think the first step should be to expose couchdb-like options. They are already well known, and there can be no discussion about their sections/names. Then a custom section could be added for pouchdb-server specific options later. Also: CouchDB (always?) directly uses a changed setting. For --in-memory, or --log, that'll require some major changes to emulate that. (Restarting the whole thing is probably the easiest way out.) That's true for --dir too, though.

@marten-de-vries
Copy link
Member Author

#81 is the first PR that actually implements part of this, everything else pointing at this was work getting everything in good shape to actually do so.

Things still on the todo list are:

  • change http port on the fly using /_config
  • change http bind address on the fly using /_config
  • documentation
  • (optional:) add command line options for CORS (using the already implemented options in /_config as fallback)
  • (optional:) add other command line options to /_config, --in-memory, --level-backend, --level-prefix & --log come to mind. (Although the last might be better off removed and replaced with just showing the log file on stdout using tail.)

marten-de-vries added a commit to pouchdb/express-pouchdb that referenced this issue Dec 17, 2014
@nolanlawson
Copy link
Member

Looks like this was fixed? Although it seems to be undocumented still.

@marten-de-vries
Copy link
Member Author

I still want to add support for the things on the todo list of my previous port. And documentation (just added that one.)

@marten-de-vries
Copy link
Member Author

With #82 the remaining TODO list is:

  • documentation
  • (optional:) add command line options for CORS (using the already implemented options in /_config as fallback)
  • (optional:) add other command line options to /_config, --in-memory, --level-backend, --level-prefix & --log come to mind. (Although the last might be better off removed and replaced with just showing the log file on stdout using tail.)

@marten-de-vries
Copy link
Member Author

Suggested config option names for the remaining CLI arguments:

{
  "pouchdb_server": { //or just 'pouchdb'? Don't really have a strong opinion.
    "level_backend": "riakdown",
    "level_prefix": "riak://localhost:8087/",
    "in_memory": true,
    "proxy": "http://localhost:5984/" //new
  }
}

I'm inclined not to add support for --user and --pass, in fact, I think it would be best to remove those options now express-pouchdb has full session/security/validation support.

@nolanlawson
Copy link
Member

+1 for removing them now that we have better configuration

@marten-de-vries
Copy link
Member Author

--log is in the same position, I removed them all in #84. (With some extra code to make express-pouchdb's log also an esthetically good replacement.)

@marten-de-vries
Copy link
Member Author

The last bit of this landed: e1611e6. Fixed now.

garrensmith pushed a commit that referenced this issue Feb 16, 2017
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

No branches or pull requests

2 participants