pebble
pebble copied to clipboard
letsencrypt
Add default configuration values
I imagine this needs some more work but I thought it was a good place to start.
I was wondering whether it was better to read the defaults from a file. Should I fix up this PR or open a new one?
I'd go ahead and keep using this one -- we just ask that you update it by pushing additional commits, not by rewriting and then force-pushing. If you'd rather start over from scratch, opening a new one is fine too! Doesn't matter much on our end :)
I was wondering whether it was better to read the defaults from a file. Should I fix up this PR or open a new one?
I'm not sure whether it is a good idea to expect the file test/config/default-config.json to be there. Until now, Pebble was working fine without that file existing if you pointed it to an alternate configuration file.
I was wondering whether it was better to read the defaults from a file. Should I fix up this PR or open a new one?
I'm not sure whether it is a good idea to expect the file
test/config/default-config.jsonto be there. Until now, Pebble was working fine without that file existing if you pointed it to an alternate configuration file.
I could swap the FailOnError for just a notification?
I personally would explicitly set the values (as in a previous version of the PR), and remove all values having the default settings from the test config files. That's pretty much incompatible to what @aarongable wrote though :)
It's not wholly incompatible -- it's fine with me if the default values are only specified in code and the default config JSON doesn't exist at all. My primary goal is that they not be specified in two places (and therefore have to be kept in sync). I just think that that solution is likely to be more confusing in the long run, as it is less "self-documenting" for consumers who do want to change values. But if it has other material advantages -- for example, not requiring the default-config.json file to exist at all -- then it is worth considering.
How about having the defaults baked in and command line flags to change them?
I'd prefer not to change the CLI of pebble at this time. A change like this imo isn't worth an API-breaking change; sticking with the existing -config flag and config file format seems like the best way forward.
One possibility here would be for getDefaultConfig to try to read the config file. If it fails to find the config file; that's fine, just return an empty config. If it finds the config file but fails to parse it, that's a problem, error out. That way someone who doesn't have the default config file on disk gets the same behavior they've always had: they have to specify everything in their config or else they'll end up with an incomplete config. But someone who wants to only customize a few things can just make sure they have the default file around, and then keep their custom config file simpler.
I'm not sure that I agree that it is a breaking change -- the behavior when no flags are passed is identical, and when specific flags are passed, it is the user's responsibility to ensure that those flags are good. Happy to be convinced otherwise, though.
That said, in order to correctly change the file to default-config.json, the README.md and docker-compose.yml also need to be updated.