Clean up configuration code #18
Conversation
bin/puppet_webhook
Outdated
| @@ -6,7 +6,13 @@ require 'webrick' | |||
| require 'webrick/https' | |||
| require 'puppet_webhook' | |||
|
|
|||
| config_file '../config.yml' | |||
| if File.exist?('/etc/puppet-webhook/server.yml') | |||
There was a problem hiding this comment.
I'd also like a command line argument which you can use to point it at.
There was a problem hiding this comment.
Do we want to add it as part of this PR or another one?
lib/puppet_webhook.rb
Outdated
| @@ -9,10 +9,15 @@ class PuppetWebhook < Sinatra::Base # rubocop:disable Style/Documentation | |||
| parsers: { 'application/json' => Sinatra::Parsers::WebhookJsonParser.new }, | |||
| handlers: { 'application/json' => proc { |e, type| [400, { 'Content-Type' => type }, [{ error: e.to_s }.to_json]] } } | |||
| register Sinatra::ConfigFile | |||
| config_file '../config.yml' | |||
| if File.exist?('/etc/puppet-webhook/app.yml') | |||
There was a problem hiding this comment.
Should we have a CONFIG_DIR setting somewhere?
There was a problem hiding this comment.
Would it provide us with anything useful?
Seems like it would just be another config option just to support the config files. Seems a bit redundant. Unless you mean to set it to one of [ static value, CLI passed arg ]
There was a problem hiding this comment.
Running from a git checkout might be easier, or as non-root user. FreeBSD has /usr/local/etc instead. There are some odd setups :)
| verify_ssl: false | ||
| public_key_path: '' | ||
| private_key_path: '' | ||
| command_prefix: 'umask 0022;' |
There was a problem hiding this comment.
I like line endings at the end of files
dhollinger
changed the title
bin/puppet_webhook
Outdated
| @@ -6,12 +6,14 @@ require 'webrick' | |||
| require 'webrick/https' | |||
| require 'puppet_webhook' | |||
|
|
|||
| confdir = File.join(__dir__, '..', 'config', 'server.yml') | |||
There was a problem hiding this comment.
confdir that's actually a file is a bit confusing.
dhollinger
force-pushed
the
config_cleanup
branch
from
b2437ec
to
606242c
Compare
App and Server default configuration files are set to look at the app root for pathing rather than a relative "../" location. The logfile setting is defaulted to /var/log/puppet-webhook/access.log
dhollinger
force-pushed
the
config_cleanup
branch
from
606242c
to
4b5bdf4
Compare
bin/puppet_webhook
Outdated
| config_file '../config.yml' | ||
| server_conf = File.join(__dir__, '..', 'config', 'server.yml') | ||
|
|
||
| if File.exist?('/etc/puppet-webhook/server.yml') |
There was a problem hiding this comment.
The only problem with a conditional like this is it's all or nothing. To override a single setting, you have to override all. My preferred solution is to load the defaults first, then load & merge the custom config if it exists.
There was a problem hiding this comment.
I agree with you on that. I'll have to look into and see if Sinatra::ConfigFile allows for merging of config from multiple files. Not sure it does.
lib/puppet_webhook.rb
Outdated
| use Rack::Parser, | ||
| parsers: { 'application/json' => Sinatra::Parsers::WebhookJsonParser.new }, | ||
| handlers: { 'application/json' => proc { |e, type| [400, { 'Content-Type' => type }, [{ error: e.to_s }.to_json]] } } | ||
| register Sinatra::ConfigFile | ||
| config_file '../config.yml' | ||
|
|
||
| app_conf = File.join(__dir__, '..', 'config', 'app.yml') |
There was a problem hiding this comment.
Same comment as above, I'd prefer to load defaults, then merge overrides.
The config_file method from Sinatra::ConfigFile can take multiple path parameters. With this in mind the default configuration path is now the first parameter and the user defined configuration path is the second argument. When loaded, if the second file contains a key that was already in the first file, that key will be overwritten with the value of the key from the second file. If the second file does not exist, then only the first file will be loaded into the settings.
Goal is to separate out the config into a
server.ymland anapp.ymland provide code that will look for default config in a common system location (/etc/puppet-webhook/<config_file>.yml) and if that is not found, then fall back to provided default config files.