(#puppethack) Add cli binary

[dhollinger]

#puppethack

Added CLI parameters and removed any unnecessary constants. Additionally, the bundled server config has been removed and the bundled app config will probably be removed as well to be replaced by either CLI options or defaults within the app.

Config loading will be replaced by a new -c --configfile option that can load a single yml file in to configure both the server AND application settings.

This would also be a good time to discuss replacing WEBrick with the generic Rack app server and adjusting code accordingly. The puppet_webhook binary is a bit bloated right now due to how WEBrick settings are loaded and there is a vulnerability tied to all versions of WEBrick (last I checked).

[coveralls]

Coverage decreased (-0.3%) to 45.105% when pulling 8c16251 on dhollinger:add_cli_binary into a734f2f on voxpupuli:master.

[coveralls]

Coverage decreased (-0.3%) to 45.105% when pulling 5465f6f on dhollinger:add_cli_binary into a734f2f on voxpupuli:master.

[ekohl]

I generally do like a config file even on the CLI. For example, It makes writing a systemd service file easier since you don't have to modify it.

Would it make sense to have a Config class so you don't need the respond to and always have a default?

[dhollinger]

I'm still planning on having a config file for the systemd unit file/init script, but that wouldn't be packaged with the gem, but rather with the RPM/Deb/Arch packages.

The Config CLI option will be used by the init/unit files to load any bundled config_file we provide. As for the respond to - if we use Sinatra's built in settings features and the config file class they provide, then the respond_to will need to be there, otherwise if the config doesn't exist it'll just error out with a NoMethodError.

[alexjfisher]

Is this the best place for a default?

[dhollinger]

It's not used anywhere else and I don't want to force users to have/create a config_file for a config option only used once.

I couldn't think of a better place, it doesn't really belong anywhere else since the configuration option is specific to the method it is in.

[ekohl]

That's what I mentioned about the config class. I also notice that it's testing for lockfile= rather than plain ```lockfile.

[dhollinger]

As far as I could tell, you have to check for 'config_option=' or :config_option= due to how Sinatra settings work and how the config_file method loads them into it.

Many of the concepts I introduced were ideas that came from https://github.com/puppetlabs/puppet-validator

[ekohl]

Would it be an idea to set (hardcoded) defaults and then load the config?

[dhollinger]

Not sure how without making every config option available globally (something I've been trying to reduce). That and we'd essentially be doing what Sinatra::ConfigFile already does and creating another class for parsing config.

This might also be a good time to discuss reducing some configuration options. We can't, nor should we, account for every potential way someone wants to use this.

[alexjfisher]

For what it's worth, settings just returns the application class itself. https://github.com/sinatra/sinatra/blob/master/lib/sinatra/base.rb#L939

[binford2k]

One thing that I've done before is to write a routine in the initializer to ensure that the settings always exist, even if they're effectively just noops. I don't know if that's a best practice or not, but it seems to work quite well, and then I don't have to sprinkle my code with conditionals.

https://github.com/puppetlabs/puppet-validator/blob/0462e21773c73682d85ce9398eb89db5a215c194/lib/puppet-validator.rb#L37-L46

[dhollinger]

@binford2k That seems like the best direction forward.

[binford2k]

Seems like a reasonable iteration forward. There are some improvements, of course, but the only (minor) red flag I saw was conditionals and defaults embedded in multiple places in the code.

[binford2k]

One thing that I've done before is to write a routine in the initializer to ensure that the settings always exist, even if they're effectively just noops. I don't know if that's a best practice or not, but it seems to work quite well, and then I don't have to sprinkle my code with conditionals.

https://github.com/puppetlabs/puppet-validator/blob/0462e21773c73682d85ce9398eb89db5a215c194/lib/puppet-validator.rb#L37-L46

[coveralls]

Coverage increased (+4.0%) to 49.315% when pulling 441e9d0 on dhollinger:add_cli_binary into a734f2f on voxpupuli:master.