(#469) Assign correct environment to node groups #479
Conversation
|
This is a proof of concept for my theory in #469 (comment) . This patch works for the described bug. The isn't complete yet and it doesn't have any new tests. Also there are other potential solutions to the problem. For example could we figure out the last used environment on a primary and use that. Or simply call |
bastelfreak
force-pushed
the
issue-469
branch
11 times, most recently
from
10594df
to
59d865a
Compare
| if $pe_install =~ String[1] { | ||
| return $pe_install | ||
| } else { | ||
| return 'production' |
There was a problem hiding this comment.
We could also return undef and then fallback to the default in the node_manager module: https://github.com/puppetlabs/puppetlabs-node_manager/blob/519bc30e0c68b11c86168868778afc37199fafc1/lib/puppet/type/node_group.rb#L124
| return 'production' | ||
| } | ||
| } else { | ||
| fail("pe_install::install::classification::pe_node_group_environment and puppet_enterprise::master::recover_configuration::pe_environment need to be set to the same value, not '${pe_install}' and ${puppet_enterprise}") |
There was a problem hiding this comment.
please let me know if you prefer fail_plan() over fail() or something else.
There was a problem hiding this comment.
I'd say fail_plan() is better for bolt plans (though, good kind name required, which is usually hard)
| @@ -94,7 +97,8 @@ | |||
| server_b_host => $replica_avail_group_letter ? { 'B' => $replica_host, default => undef }, | |||
| internal_compiler_a_pool_address => $replica_avail_group_letter ? { 'A' => $replica_host, default => undef }, | |||
| internal_compiler_b_pool_address => $replica_avail_group_letter ? { 'B' => $replica_host, default => undef }, | |||
| peadm_config => $peadm_config | |||
| peadm_config => $peadm_config, | |||
There was a problem hiding this comment.
I think that puppet-lint doesn't detect the missing comma is a bug: https://github.com/puppetlabs/puppet-lint/issues/215
bastelfreak
force-pushed
the
issue-469
branch
6 times, most recently
from
7ec995e
to
954c50f
Compare
bastelfreak
changed the title
bastelfreak
changed the title
This checks if a user configured a environment in pe.conf. If that's the case, it will be used for the PEADM-specific node groups. Otherwise we fall back to production. This fixes a timing issue discovered in puppetlabs#469. In situations where the PE infra isn't running in production, we cannot assume that a production environment exists. And a node group can only reference classes from the environment the node group belongs to.
|
I added unit tests and everything passes. Can you please review this? |
|
I seem to vaguely remember that |
|
@timidri I didn't find anything related in the docs about it. |
bastelfreak
merged commit e6092ff
into
puppetlabs:support-custom-environment
This checks if a user configured a environment in pe.conf. If that's the case, it will be used for the PEADM-specific node groups. Otherwise we fall back to production.
This fixes a timing issue discovered in #469. In situations where the PE infra isn't running in production, we cannot assume that a production environment exists. And a node group can only reference classes from the environment the node group belongs to.
Summary
Provide a detailed description of all the changes present in this pull request.
Additional Context
Add any additional context about the problem here.
Related Issues (if any)
Mention any related issues or pull requests.
Checklist
Changes include test coverage?
Have you updated the documentation?