Rethink $code_manager_auto_configure

[bastelfreak]

Describe the Bug

Good morning. Maybe this is a bug, maybe a feature request, I'm not sure.

In peadm::subplans::install and in peadm::install we have the following parameter signature:

Optional[Boolean] $code_manager_auto_configure = true,

The value has a default parameter that's not undef. The datatype is undef. We cannot pass undef to it. When we call the plan and don't set the parameter, or if we pass undef, the default from the plan, true, is used. A fix would be to remove the Optional and make it a boolean:

Boolean $code_manager_auto_configure = true,

But does that make sense? $r10k_remote, $r10k_private_key_file and $r10k_private_key_content are optional. So by default we enable code manager but don't configure any repository. This will lock r10k, we're not able to call r10k directly anymore. But that might be desired for bootstrapping. We're now forced to add a repo via the PE console or disable the code manager.

Expected Behavior

I think the default value for $code_manager_auto_configure should be undef. People can explicitly enable or disable it. If $r10k_remote and $r10k_private_key_file or $r10k_private_key_content are set and $code_manager_auto_configure isn't false, we should enable code manager.

[bastelfreak]

I hacked a possible fix/enhancement in #352. Please let me know what you think about it

[ragingra]

Good evening Tim!

I’ve done a little asking around about this and believe that this is fine as is. The reason it's there is that you can't add a replica or a compiler unless code manager is on. When we made the param configurable we left the default as true to not interfere with other users. https://github.com/puppetlabs/puppetlabs-peadm/pull/341

If we were to set it to undef, the value would be automatically set to false which might cause issues. https://www.puppet.com/docs/puppet/8/lang_data_boolean.html#lang_data_boolean-automatic-conversion-boolean

Let me know if that’s of any help!

[bastelfreak]

believe that this is fine as is
I don't think so. At least the datatypes + default value don't make any sense and we even have a puppet-lint plugin for this:

• puppet-lint-optional_default-check
• which is pulled in by voxpupuli-puppet-lint-plugins
• which is pulled in by PDK and Vox Pupuli and considered current best practice

With the current setup I don't see an easy option for people to configure code manager later on. I cannot deploy a temporary control repo with r10k because it's locked. And values from the classifier overrule hiera. Even if I can bootstrap code somehow, I cannot change code manager because it's hardcoded in the node group.

And not every setup has a replica/compiler.

I think the logic in the plan could be reworked:

• switch the default for $code_manager_auto_configure to undef
• if it's undef and $r10k_remote and $r10k_private_key_file or $r10k_private_key_content are set and $code_manager_auto_configure isn't false, enable it
• if a replica and or a compiler should be configured and $code_manager_auto_configure is undef, enable it

[CoMfUcIoS]

So to understand this right, we shouldn't configure code manager automatically if it isn't required by installation.

So a logic like the following is what we need here?

If DR is required configure code manager, else let the user decide through the code_manager_auto_configure if he/she wants us to configure it

[bastelfreak]

@CoMfUcIoS yes, that's essentially what my PR is about. Make the setup of code manager optional. The PR doesn't change the default behaviour. It just enables users to not setup code-manager during the peadm::install plan.