Fixes #28530: Combine post install hooks into a single one #436
Conversation
There was a problem hiding this comment.
Kafo has the interesting thing that it always loads #{installer_dir}/hooks (source). If we always load the basic hooks as well, then we can change the installer_dir to ./ and leave out ./hooks in hook_dirs. I initially implemented it this way to keep the separation, but it also requires workarounds (like 85f84a1). My goal was always to have installer_dir: . in katello.yaml and foreman-proxy-content.yaml.
Speaking of foreman-proxy-content, that probably needs the same treatment.
Other implications of this are:
- Introduces
--reset-foreman-dbto the Katello scenario (and fpc if that is also modified). Has a lot in common with the--resetoption Katello now has. Probably a good chance for unification. - Introduces
--detailed-exitcodes(a good thing IMHO) katello/hooks/pre/21-check-hammer-credentials.rbis now a duplicate ofhooks/pre/20-check-hammer-credentials.rbkatello/hooks/pre/25-remove_apache_from_foreman_group.rbis now a duplicate ofhooks/pre/25-remove_apache_from_foreman_group.rb
Overall I really like the intent of this change.
I've been thinking about (unit) testing hooks. ekohl@1d80a8f#diff-ffd29a2cef868d990ba1fe2337fe9b23 can serve as inspiration.
|
Updated and filed an issue to resolve --reset and --reset-foreman-db (https://projects.theforeman.org/issues/28533) as a follow up. The code change seemed big enough to warrant it's own PR. |
Fair enough. Put some comments there. |
|
Thanks for the feedback, updated |
There was a problem hiding this comment.
Just thinking out loud. If you have multiple extensions you include, could you have something like:
module HookContextExtension
def post_message
# ...
end
end
Kafo::HookContext.send(:include, HookContextExtension)Now in a later hook if you want to override this:
module CustomizedMessagesExtension
def post_message
# ...
end
end
Kafo::HookContext.send(:include, CustomizedMessagesExtension)I'm wondering if we can avoid the whole MessageHelper class. It's also rather ugly that it's in the Kafo namespace.
| end | ||
|
|
||
| # Puppetmaster? |
There was a problem hiding this comment.
Do we announce any other service that is running? Seemed odd to me. We are installing Foreman, that should be all we point users to and they can go figure out where things are running if needed from the smart proxy attached.
ehelms
force-pushed
the
fixes-28530
branch
3 times, most recently
from
7daa6ac
to
847a51d
Compare
config/foreman-proxy-content.migrations/191218160614-update-hook-dirs.rb
Outdated
Show resolved
Hide resolved
ehelms
force-pushed
the
fixes-28530
branch
2 times, most recently
from
324e2e0
to
64c689a
Compare
|
Updated and added test back with fixture updates |
| @@ -6,7 +6,7 @@ def success_message | |||
|
|
|||
| def server_success_message(kafo) | |||
| success_message | |||
| say " * <%= color('Katello', :info) %> is running at <%= color('#{kafo.param('foreman', 'foreman_url').value}', :info) %>" | |||
| say " * <%= color('Foreman', :info) %> is running at <%= color('#{kafo.param('foreman', 'foreman_url').value}', :info) %>" | |||
There was a problem hiding this comment.
This changes the message from "Katello" to "Foreman" (while keeping "Katello" in the devel scenarios), which I think is fine, but might be unexpected by users? Should this be the scenario name instead?
There was a problem hiding this comment.
If it uses the scenario name, then it might also solve the downstream branding and remove the need for message helpers.
There was a problem hiding this comment.
The scenario name usage makes the assumption that thing running is always congruent with the name. I don't want to confuse users further with things like "Luna is running at" "Foreman Proxy with Content is running at". The software that's running in this case is Foreman (with plugins).
There was a problem hiding this comment.
Not for this PR, but we can think about storing that in the scenario. New fields can be added and I think I'd rather deal with that in scenarios than in ruby code.
This moves to a single post install hook to handle all scenarios. This introduces the message helpers to Foreman hooks and starts to provide the hook extensions Katello uses.
This moves to a single post install hook to handle all scenarios.
This introduces the message helpers to Foreman hooks and starts
to provide the hook extensions Katello uses.
This starts the road to a single hooks directory that handles all scenarios properly. Further it introduces a few concepts in Katello's hooks to Foreman's to pave the way for more changes. I did not want to re-write it all in one go but rather baby step changes in. Thus, I figured the message helpers and post install hook being combined was a good first.