Skip to content

Rethink $code_manager_auto_configure #351

Closed
@bastelfreak

Description

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.

Activity

added a commit that references this issue on May 19, 2023

(puppetlabs#351) code_manager: Switch default to `undef`

94871f3
bastelfreak

bastelfreak commented on May 19, 2023

@bastelfreak
CollaboratorAuthor

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

added 3 commits that reference this issue on May 19, 2023

(puppetlabs#351) code_manager: Switch default to `undef`

a7f4831

(puppetlabs#351) code_manager: Switch default to `undef`

f5e5662

(puppetlabs#351) code_manager: Switch default to `undef`

fe70fa1
ragingra

ragingra commented on May 19, 2023

@ragingra
Member

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

bastelfreak commented on May 22, 2023

@bastelfreak
CollaboratorAuthor

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:

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
added 2 commits that reference this issue on Aug 14, 2023

(puppetlabs#351) code_manager: Switch default to `undef`

5a2a25a

(puppetlabs#351) code_manager: Switch default to `undef`

e08c4b5
added a commit that references this issue on Sep 18, 2023

(puppetlabs#351) code_manager: Switch default to `undef`

6006175
CoMfUcIoS

CoMfUcIoS commented on Sep 19, 2023

@CoMfUcIoS
Contributor

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

added a commit that references this issue on Sep 27, 2023

(puppetlabs#351) code_manager: Switch default to `undef`

e2c4660
bastelfreak

bastelfreak commented on Sep 27, 2023

@bastelfreak
CollaboratorAuthor

@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.

5 remaining items

Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions

      Rethink $code_manager_auto_configure · Issue #351 · puppetlabs/puppetlabs-peadm