Fixes #33577 - Only enable Foreman Puppet plugin with Foreman#723
Fixes #33577 - Only enable Foreman Puppet plugin with Foreman#723ekohl merged 1 commit intotheforeman:developfrom
Conversation
|
I chose to change the existing migration so we don't end up in a bad state, but I wonder if we should instead create another migration as well that disables it if foreman itself is disabled. Users who are affected couldn't have upgraded to 3.0.0 (https://community.theforeman.org/t/foreman-proxy-3-0-installation-fails-with-parameter-config-file-group-expects-a-string-value/25438) so perhaps it's ok, but it may not be the cleanest. |
wbclark
left a comment
wbclark
left a comment
There was a problem hiding this comment.
I would personally vote for a new migration that disables the plugin if foreman itself is disabled, for the reason that it would reduce the number of accidentally broken configurations in the wild (vs. the solution here which stops adding to them but doesn't correct those which already exist)
Perhaps the difference is minimal or low impact but I like the practice of trying to reduce that drift wherever possible, as that can prevent future problems.
If the user passed in --no-enable-foreman then the Foreman plugin should not be enabled. The same is true for the CLI side of it. The add-cli migration is changed to respect an already present option that is false in order to properly test this change.
ekohl
force-pushed
the
33577-fix-puppet-migration
branch
from
1b39736 to
ce89a30
Compare
|
Updated. Now also contains the CLI part of it (as reported by the user) and tests. I've started a new pattern for the tests and want to look at using for all other tests. The reporting of what failed is much clearer to me. We can also pull the |
3 participants
If the user passed in
--no-enable-foremanthen the Foreman plugin should not be enabled.