Bug #7270
unify global options with face and action options...
| Status: | In Topic Branch Pending Review | Start date: | 04/28/2011 | |
|---|---|---|---|---|
| Priority: | Low | Due date: | ||
| Assignee: |  Kelsey Hightower |
% Done: | 0% |
|
| Category: | Faces | |||
| Target version: | 2.7.x | |||
| Affected Puppet version: | development | Branch: | https://github.com/khightower/puppet/commits/feature/master/7270 | |
| Keywords: | ||||
| Votes: | 1 |
Description
At the moment we handle global options for faces in a … different way. They are basically the old fashioned application options, kind of vaguely repurposed to do something approximating the right thing. We should unify the behaviour of those, ideally along with options derived from our configuration system, and make them all behave consistently.
This would naturally want to involve unification of the DSL for declaring options, and in turn for introspection of them: that would make it practical to have them in the help output, and so forth. Which is desirable, especially round the area of option validation.
It also means we can unify error handling, so that we don’t have some classes of validation resulting in good help output, and others resulting in bad help output, just because they are handled by different substrates in the product.
Related issues
History
Updated by James Turnbull about 1 year ago
- Status changed from Unreviewed to Needs Decision
Updated by Nigel Kersten about 1 year ago
Daniel, is it possible to get a summary of the negative aspects of the current implementation, particularly as it would affect Face developers and users?
Updated by Daniel Pittman about 1 year ago
- Tracker changed from Refactor to Bug
Daniel, is it possible to get a summary of the negative aspects of the current implementation, particularly as it would affect Face developers and users?
At the moment the “global” options are implemented by using the legacy application option processing system. This means that:
- can’t be introspected, unlike the other options
- don’t have the same hooks for documentation, etc
- don’t show up in the help output
- have a block that can do arbitrary things, which works once, rather than like other options do
That said, the only real way to add a “global” option is to hack into the face_base application, or their wrapper application, so end users are really not that likely to run into this. They probably won’t be running into anything except the lack of documentation of it…
The biggest problem is that they are not uniform in a lot of ways, like the introspection / help thing, so as we add more features they won’t be found in the few global options we have…
Updated by Nigel Kersten about 1 year ago
- Status changed from Needs Decision to Accepted
- Assignee deleted (
Nigel Kersten) - Priority changed from Normal to Low
Updated by Daniel Pittman 10 months ago
- Target version changed from 2.7.1 to 2.7.x
Updated by K Hightower 9 months ago
- Branch set to https://github.com/khightower/puppet/commits/feature/master/7270
I am submitting a patch that takes a “small” step forward by exposing non-standard configuration parameters set in puppet.conf, the cli, or any thing that uses Puppet::Util::Settings.set_value. This allows Puppet Faces to reference custom parameters set in puppet.conf . Please review my branch and let me know if I am heading in the right direction.
Updated by Daniel Pittman 9 months ago
Kelsey Hightower wrote:
I am submitting a patch that takes a “small” step forward by exposing non-standard configuration parameters set in
puppet.conf, the cli, or any thing that usesPuppet::Util::Settings.set_value. This allows Puppet Faces to reference custom parameters set inpuppet.conf. Please review my branch and let me know if I am heading in the right direction.
Hey. I took a look, and it feels like a reasonable enough approach. I think if that is wired up so that it works correctly for configuration file options – ideally, nested, so you can create arbitrary sections, and options in those sections – then it would be acceptable. Given how core this is, we will need to have tests against it.
Updated by K Hightower 9 months ago
Cool, I will start working on some tests for this.
Updated by K Hightower 9 months ago
I have updated my branch with some tests and updated the patch to support arbitrary sections and options, including nested ones.
Updated by James Turnbull 9 months ago
- Status changed from Accepted to In Topic Branch Pending Review
Updated by K Hightower 9 months ago
I have updated my puppet-dashboard-face to use this patch. This should provide an example use case for accessing paramaters from puppet.conf.
https://github.com/khightower/puppet-dashboard-face
One thing I need to figure out is how to address settings via section names. Right now I am prefixing my parameters with dashboard_face_ even though they are under a different section.
I am doing this in puppet.conf:
[dashboard]
dashboard_face_adapter = mysql
dashboard_face_host = 127.0.0.1
dashboard_face_database = dashboard_production
dashboard_face_username = dashboard
dashboard_face_password = dashboard
I am using the settings like this:
@user = options[:username] || Puppet.settings[:dashboard_face_username]
@password = options[:password] || Puppet.settings[:dashboard_face_password]
@database = options[:database] || Puppet.settings[:dashboard_face_database]
@host = options[:host] || Puppet.settings[:dashboard_face_host]
@adapter = options[:adapter] || Puppet.settings[:dashboard_face_adapter]
I would like to do this:
Puppet.settings["dashboard"][:database]
Updated by Nigel Kersten 9 months ago
Ugh, that’s not great you’re being forced into that.
I’m pretty sure we do currently have a uniqueness constraint across all options, regardless of the block they’re in unfortunately.
Updated by James Turnbull 8 months ago
- Assignee set to K Hightower
Kelsey – are duplicate blocks merged or overridden?
Updated by K Hightower 4 months ago
- Assignee changed from K Hightower to Kelsey Hightower