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

Add default configuration values #316

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

AdamBark
Copy link

Address issue #297

@AdamBark
Copy link
Author

I imagine this needs some more work but I thought it was a good place to start.

Copy link
Contributor

@aarongable aarongable left a comment

Choose a reason for hiding this comment

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

Thanks for putting this together! This is really solid. My three in-line comments are about ways to improve this current approach; I also have a different approach to propose which I believe would address all of them:

  1. Move /test/config/pebble-config.json to /test/config/default-config.json
  2. Define a const string which is "/test/config/default-config.json"
  3. Use that const string as the default value of the -config flag
  4. Have the getDefaultConfig method simply read that config file

That way the default values don't have to be duplicated in two places (and in two formats).

@@ -29,6 +29,17 @@ type config struct {
}
}

func setConfigDefaults() (config) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: because this is creating a new config object, rather than setting values on a pre-existing config object, I'd call it getDefaultConfig rather than setConfigDefaults.

Copy link
Author

Choose a reason for hiding this comment

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

That's what you get for not updating your function names when you change what it does 🤦

conf.Pebble.PrivateKey = "test/certs/localhost/key.pem"
conf.Pebble.HTTPPort = 5002
conf.Pebble.TLSPort = 5001
return conf
Copy link
Contributor

Choose a reason for hiding this comment

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

There are two values (ocspResponderUrl and externalAccountBindingRequired) which don't get initialized here. Sure, their defaults are also their zero-values, but being explicit is good.

@@ -29,6 +29,17 @@ type config struct {
}
}

func setConfigDefaults() (config) {
var conf config
conf.Pebble.ListenAddress = "0.0.0.0:14000"
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a comment showing readers that these values are identical to the values in /test/config/pebble-config.json, and that they should be kept in sync.

@AdamBark
Copy link
Author

AdamBark commented Jul 27, 2020

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?

@aarongable
Copy link
Contributor

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 :)

@AdamBark AdamBark requested a review from aarongable July 27, 2020 17:57
cmd/pebble/main.go Outdated Show resolved Hide resolved
Co-authored-by: Aaron Gable <[email protected]>
@AdamBark AdamBark requested a review from aarongable July 27, 2020 21:15
aarongable
aarongable previously approved these changes Jul 28, 2020
Copy link
Contributor

@aarongable aarongable left a comment

Choose a reason for hiding this comment

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

LGTM; requesting a second pair of eyes.

@aarongable aarongable requested a review from a team July 28, 2020 17:24
@felixfontein
Copy link
Contributor

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.

@AdamBark
Copy link
Author

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 could swap the FailOnError for just a notification?

@felixfontein
Copy link
Contributor

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 :)

@aarongable
Copy link
Contributor

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.

@AdamBark
Copy link
Author

How about having the defaults baked in and command line flags to change them?

@aarongable
Copy link
Contributor

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.

If default config exists try to read it into the config structure else
return a blank config.
felixfontein
felixfontein previously approved these changes Jul 31, 2020
Copy link
Contributor

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

The current approach is reasonable. The only problematic part is that the location of the config file is changed, which is a breaking change. Following semver, this needs to be released as 3.0.0.

@aarongable
Copy link
Contributor

aarongable commented Jul 31, 2020

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.

@AdamBark AdamBark removed the request for review from a team August 5, 2020 16:16
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.

3 participants