Fixes #33687 - Add warning not to directly edit settings.py

[wbclark]

Short term static alternative to #210

[ekohl]

I think this is a very long header and I generally dislike those. In puppet-foreman we have https://github.com/theforeman/puppet-foreman/blob/master/templates/_header.erb and IMHO that's plenty long.

[ekohl]

Suggested change

	# Module: pulpcore #
	# Module: <%= scope['module_name'] %> #

[wbclark]

Would $module_name ever take on another value? If not, then why not simply hard code it?

Note in the generic version I intend to build, it would be dynamic so that it could be used by other modules. That would also have an overrideable $installer_name so that it can say, e.g. File managed by foreman-installer one line above.

[ehelms]

Nah, I think it will always be the module name based on the metadata and thus no worry that it might change for right now.

[ekohl]

However, if you do want to go to a generic header it makes sense to think about it. For example, the closing # that you add now becomes a lot harder. That's why I'd advise to leave that part out. I should have mentioned that in my initial review.

My reasoning is that a change now will lead to a service restart. Restarting all services just for a new comment in a config file is a bit of a waste. So I'd prefer to get it right the first time.

[ehelms]

I think I see what you are saying... the trailing # is harder to calculate than if there was none. So if that part was dropped, when the generic solution is implemented it matches this closer and is easier to implement. We do not necessarily need the aesthetic.

[wbclark]

Understood. With that said, I already solved that issue in my proof of concept for the more general case:

https://github.com/theforeman/puppet-pulpcore/pull/210/files#diff-893d976ae6fa01e560858f0185fdcfad47f8550e71e8a510fc7a8135ca8e03e6R16

[ekohl]

Is this part really needed? The user may not have explicitly thought about it so how are they going to link it back?

[wbclark]

I think it's important to provide some explanation to the user so that they understand why they are asked to not make edits.

If there is a warning not to make edits, but no explanation, a confident user might say "oh, I already understand it won't be persistent, so this warning doesn't apply to me. I just want to try out that other tasking system..."

I agree it's difficult to phrase it in such a way that is equally applicable to users of puppet, foreman-installer, satellite-installer, orcharhino, etc. Necessarily, the explanation will be vague, and this is the problem that I intend to solve by allowing custom strings in hiera.

But in the meantime, with the upcoming release introducing several new config file entries that can easily be broken in unexpected ways, I argue that a vague explanation is still better than no explanation at all.

[ehelms]

I think this is a fair start, and the systematic change being worked on will be a better long term solution. For now this achieves the primary objective of helping users understand they should not be editing. I suppose we will learn over time if it actually works.

[ehelms]

@wbclark I think there is at least one suggestion to address here with the dynamic module name

@ekohl what are your current thoughts on this change going in?

[wbclark]

@wbclark I think there is at least one suggestion to address here with the dynamic module name

Thanks.

I think that static module name should be fine for this PR since it will never change.

As I understood the suggestion from @ekohl it was a recommendation to think about the future where this type of logic could be used by any module. I have done that, and it is solved in #210

So I would propose that we:

1. merge this as is, as it provides some warning to users now
2. re-implement Add customizable warning not to directly edit settings.py #210 as a PR to an existing library or as a new module, to allow downstream customization and standardize across modules in the future

OR

1. merge Add customizable warning not to directly edit settings.py #210 now, as it does everything this does, with the additional feature that the strings can be overwritten in hiera for messages that are more targeted by use case
2. later re-implement that one as a PR to an existing library or as a new module, to standardize across modules in the future

The upside of the 2nd approach would be the additional customization it allows, which solves the problem of documenting/commenting for pure puppet deployments vs. foreman installer vs. downstream; the downside would be that a future breaking change becomes more likely if the customizable form is merged now, then goes through additional reviews and changes during externalization

[wbclark]

After further thought and discussion, I pushed an update with the following changes:

1. Created Redmine #33687 and linked it for tracking purposes
2. Changed to <%= scope['module_name'] %> for consistency with other module headers
3. Removed closing # characters. I added these in Add customizable warning not to directly edit settings.py #210 because I intend to build that out to support any comment syntax, including one that might have opening and closing tags for comments. While I also like the visual style these provide, it's not important to the core functionality of this feature in this module.

[ekohl]

I still feel this is quite a heavy text. Some inline comments to make it a bit smaller.

I do like the reference to the documentation.

[wbclark]

@ekohl This is updated with your requested changes.

Longer term, I still prefer a solution using custom strings that can be overwritten in hiera, so that foreman-installer, satellite-installer, orcharhino, or any other tool using this module can provide warnings that are tailored to their use case.

For example, satellite-installer could link directly to an article on access.redhat.com describing how to (re-)configure Pulpcore using satellite-installer.

[ekohl]

If that's your goal, why not copy what we have in dhcp? There's a parameter for the header:
https://github.com/theforeman/puppet-dhcp/blob/01c4c5cfba3813dfa0ae7daa2282308035b8c027/manifests/init.pp#L44
This is written here:
https://github.com/theforeman/puppet-dhcp/blob/01c4c5cfba3813dfa0ae7daa2282308035b8c027/templates/dhcpd.conf.erb#L1-L3
We then override this header in Hiera:
https://github.com/theforeman/foreman-installer/blob/bef5bd620a5b1494d3b5779030bc6b29e861645c/config/foreman.hiera/common.yaml#L3-L9

satellite-installer can then ship another hiera file (https://github.com/theforeman/foreman-installer/blob/bef5bd620a5b1494d3b5779030bc6b29e861645c/config/foreman-hiera.yaml#L11-L12) and override this again.

[wbclark]

If that's your goal, why not copy what we have in dhcp?

Thanks, that is neat.

Quite a bit simpler, if not as robust, as what I had in #210