Add new configuration option for puppet classes #338

ehelms · 2021-08-13T18:14:32Z

This builds on the initial proposal from this RFC (https://community.theforeman.org/t/defaulting-puppet-to-off-in-the-katello-scenario/14553) and further thoughts within the RFC (https://community.theforeman.org/t/defaulting-puppet-to-off-in-the-katello-scenario/14553/8).

This approach is to apply the idea of keeping scenario configuration separated from the parameter value storage. The benefits and aim of this solution over a complete version 2 answer file is:

This change is progressive and backwards compatible. Scenarios can add this new section to the configuration file and take advantage of the new features.
This model allows adding new features over time as new keys can be added to the configuration hash (e.g. include params, controlling enablement state)
By keeping this in the configuration file, the answers file can remain in standard hiera yaml format. This means no custom backend or manipulation of the parameters in the answers file to allow the hiera hierarchy to continue to work.
This change is much smaller than an over haul to add support for a V2 answer file would be and thus the risk is much lower to existing projects.

The initial features this adds are:

Ability to specify if a puppet class should be presented to the user as something than can be disabled. If can_disable is set to false, the user is never presented with a configuration option to disable it.
Ability to specify parameters that should not be presented to a user as a CLI option. Then the scenario can hide parameters that either clutter the help output or are not meant to be configured by a user in an installer setup.

Example: Set foreman to not be able to disable and exclude the version and db_manage parameter:

:classes:
  :foreman:
    :enabled: true
    :can_disable: false
    :exclude:
      - version
      - db-manage

Why a dedicated can_disable flag instead of a multi-state enabled field (e.g. true/false/always) ?

Attempting to use a single field for enabled did not easily cover all the states we need. Those being:

Puppet class is enabled by default, but can be disabled
Puppet class is disabled by default, can be enabled and then later disabled
Puppet class is enabled by default but cannot be disabled
Puppet class is disabled by default, can be enabled but once enabled cannot be disabled

The last condition here not being covered by the original RFC proposal of using 'always'. This separate field let's us build the entire matrix of states and cleanly separates the state (enabled/disabled) and capability (can be disabled).

Why not add an include field right now?

Since all parameters are included by default, we need only to currently specify which parameters to exclude.

Future Thinking

Looking ahead, this configuration area could be used to control what set of modules are available and what enabled state they are in by default. That would then allow the answers file to be simplified down to only specifying classes and parameters that override the defaults provided by the modules. Both slimming down the answer file and matching it closer to the other hiera configuration files.

Previously the PuppetModule class would report whether a module was enabled or reach out to the configuration class to ask if itself was enabled. This created a tightly coupled relationship between PuppetModule and the configuration object and this change centralizes that knowledge into the PuppetModule class.

This adds configuration of class enablement into the configuration file along with the ability to specify whether a class can be disabled or not. This continues to support enable declarations within the answers file but is superseded by anything declared in the configuration file. This separates the scenario configuration, from the actual puppet class parameters found in the answers file. Further, this allows the answers file to focus on parameters and being part of the standard hiera heirarchy while declaring properties of the classes within the configuration. This should be thought of splitting scenario properties, aspects of a scenario a user should not change, from the elements that should be configurable via user input.

Excluded parameters can be defined for a given Puppet class that is included such that the parameter is never presented to the user as a command line option. These excluded parameters still appear in the answers file and can be set through hiera.

ehelms · 2021-08-13T18:16:41Z

lib/kafo/configuration.rb

@@ -43,6 +43,7 @@ class Configuration
      ScenarioOption::KAFO_MODULES_DIR          => nil,
      ScenarioOption::CONFIG_HEADER_FILE        => nil,
      ScenarioOption::DONT_SAVE_ANSWERS         => nil,
+      ScenarioOption::CLASSES                   => {},


The standard nomenclature we have been using is modules; but what we really seem to specify everywhere are puppet classes not puppet modules. Thus I decided to aim for using the correct representation.

Provide the ability to specify if a class is enabled or disabled through the classes configuration option rather than through the answer file. This is a progressive option, as it takes precedence over values in the answer file but continues to respect answer file if no data is found in the classes option.

ehelms · 2021-08-26T01:31:37Z

Another option for this, is to introduce a new dedicated file for handling puppet class configuration options such as those introduced here. That file, being specific to a scenario, could then be saved to a different location (such as /var/lib/installer) to keep it separate from "users" configurations and rather serve as something more akin to "internal" configuration or database like entity.

evgeni · 2021-09-03T10:35:32Z

test/acceptance/kafo_configure_test.rb

+      it 'shows enable flag if class is disabled' do
+        config = YAML.load_file(KAFO_CONFIG)
+        config[:classes] = {:testing => {:can_disable => false}}
+        File.open(KAFO_CONFIG, 'w') do |file|
+          file.write(config.to_yaml)
+        end
+
+        answers = {'testing' => false}
+        File.open(KAFO_ANSWERS, 'w') do |file|
+          file.write(answers.to_yaml)
+        end
+
+        code, out, err = run_command '../bin/kafo-configure --help'
+        _(code).must_equal 0, err
+        _(out).must_include "--enable-testing"
+      end


does that mean that when I've run that-installer --enable-testing once, the next run will error out because --enable-testing is not an option anymore? I can see some (a|se)nsible users cry about that.

That is a fair point. My thought was, if the option does nothing, why include it. There are a couple ideas:

If a module is already enabled and cannot be disabled, promote it to --full-help to get it out of the users way but still exist

I have been thinking of a better option for ansible-style users, where users can provide a file as input that maps to installer parameters. That would be a nicer interface for those type of users. Though it may suffer from the same problem (no such option).

Demoting it to full-help sounds good to me.

While we can (and should) make the Ansible life easier by making it understand the installer, we also have users out there who have a 50+ page PDF "how to install/upgrade $project" and they will re-use the same installer parameters on every installer invocation.

Is that (repetition of the params on every invocation) needed? Of course not.
Will the users be totally confused that they have feature X enabled, and yet the installer tells them there is no such thing as feature X (which is how they will read the absence of --enable-X)? Absolutely.
(As me how I know.)

evgeni · 2021-09-03T11:54:37Z

lib/kafo/kafo_configure.rb

+        elsif !mod.enabled?
+          app_option d("--enable-#{mod.name}"), :flag, "Enable '#{mod.name}' puppet module (current: #{mod.enabled?})"
+        end


I guess https://github.com/theforeman/kafo/pull/338/files#r701787011 should rather have been here, but 🤷‍♀️

ehelms added 3 commits August 12, 2021 20:54

theforeman-bot added Needs testing Not yet reviewed labels Aug 13, 2021

ehelms commented Aug 13, 2021

View reviewed changes

ehelms force-pushed the new-enable-config branch from ba87491 to ee2c6ff Compare August 24, 2021 14:35

ehelms mentioned this pull request Aug 25, 2021

Externalize answer defaults theforeman/foreman-installer#718

Draft

evgeni reviewed Sep 3, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new configuration option for puppet classes #338

Add new configuration option for puppet classes #338

ehelms commented Aug 13, 2021 •

edited

Loading

ehelms Aug 13, 2021

ehelms commented Aug 26, 2021

evgeni Sep 3, 2021

ehelms Sep 3, 2021

evgeni Sep 3, 2021

evgeni Sep 3, 2021

Add new configuration option for puppet classes #338

Are you sure you want to change the base?

Add new configuration option for puppet classes #338

Conversation

ehelms commented Aug 13, 2021 • edited Loading

ehelms Aug 13, 2021

Choose a reason for hiding this comment

ehelms commented Aug 26, 2021

evgeni Sep 3, 2021

Choose a reason for hiding this comment

ehelms Sep 3, 2021

Choose a reason for hiding this comment

evgeni Sep 3, 2021

Choose a reason for hiding this comment

evgeni Sep 3, 2021

Choose a reason for hiding this comment

ehelms commented Aug 13, 2021 •

edited

Loading