Fixes #36809 - Do not clobber answers provided on the command line wi…

[ehelms]

…th --certs-regenerate

Problem:

foreman-installer --certs-state Texas --certs-regenerate True

The installer will see and run as if the state value was changed to Texas, regenerating certificates. However, because kafo.config.app.answers returns the answers file and not the compiled params, what gets written back to the answers file is the original answers at runtime. Thus the answers file now have the previous value (e.g. North Carolina) and the next installer run will trigger a certificate update under the hood.

[ehelms]

I agree this is a bit ugly, calling into a private method. My goal here is to patch the problem first with a fix that can be cherry picked cleanly backward. The alternative is to add methods to kafo and then release a patch release and also a patch to the installer for older releases.

[ehelms]

This is intended to be a patch that fixes the issue, and I think what needs to follow is a real fix to the way we regenerate certificates which is now possible that we have gotten rid of the RPM part.

[ekohl]

Some notes: it was introduced in e28a519 and we shouldn't store --certs-regenerate but it should be passed to Puppet.

I'm now debating options, but can't think of any right now. I'll get back to this.

[ehelms]

[test foreman-installer]

[ehelms]

@ekohl This is still a pretty ugly problem that it can cause, do you have any additional thoughts or review?

[ehelms]

@ekohl Do you have any additional ideas or can we move forward with this change to prevent this issue as-is for users?

[ekohl]

My issue with the current implementation is that --noop or --dont-save-answers isn't respected: it always saves the answers.

I'm looking at https://github.com/theforeman/kafo/blob/master/doc/kafo_run.png and I'm wondering if we should take a different approach:

• In pre_commit we ensure the value is false in the answers file
• Kafo saves the answers
• In pre we set the answer back to what it was supposed to be

I don't know what the best mechanism is to store data between hooks. Our store_custom_config is probably the best we have.

[ehelms]

My issue with the current implementation is that --noop or --dont-save-answers isn't respected: it always saves the answers.

I'm looking at https://github.com/theforeman/kafo/blob/master/doc/kafo_run.png and I'm wondering if we should take a different approach:

• In pre_commit we ensure the value is false in the answers file
• Kafo saves the answers
• In pre we set the answer back to what it was supposed to be

None of this worked. And you are correct -- the current implementation saves the answers and that is why it's not working entirely correctly. I tried a variety of ideas using the pre state and could not get things to land in the correct state.

The approach I have proposed here modifies this to only reset the single parameter back to false rather than resetting all the answers back.

Ideally we get rid of the regenerate parameter, I think that is a bigger change and my aim here is to fix the existing problem in a minimal way.

[ekohl]

Let's at least respect the --noop and --dont-save-answers flags and avoid storing the answers then. Then I think this is OK to merge.

[ehelms]

Let's at least respect the --noop and --dont-save-answers flags and avoid storing the answers then. Then I think this is OK to merge.

Accepted your update

[ehelms]

@ekohl Are you good with this updated form?

[ehelms]

@ekohl I would still like to get this localized fix in to prevent this issue for users and deal with the larger re-factor design issues as a follow up.