Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #30507: Use enabled hook helpers for Katello and Foreman #546

Merged
merged 1 commit into from
Aug 27, 2020

Conversation

ehelms
Copy link
Member

@ehelms ehelms commented Jul 28, 2020

No description provided.

hooks/pre/10-reset_data.rb Outdated Show resolved Hide resolved
katello/hooks/pre/15-check_java.rb Outdated Show resolved Hide resolved
Comment on lines +21 to 23
if app_value('certs_update_server_ca') && !katello_enabled?
fail_and_exit("--certs-update-server-ca needs to be used with katello", 101)
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part should really be a pre_commit hook IMHO. In pre the answers are already saved to disk. You should fail on invalid user input in pre.

It should also check if the certs module is enabled, rather than katello, right? It checks if the certs params are passed in and I think that server_ca_cert can also be used on a content proxy.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

File an issue if you don't mind, touching our certs code requires a Friday :P

@ehelms
Copy link
Member Author

ehelms commented Jul 31, 2020

Added candlepin_enabled? helper

@ehelms
Copy link
Member Author

ehelms commented Aug 17, 2020

This could use another round of review

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like I had comments that I failed to submit

katello/hooks/pre_commit/14-cdn_setting.rb Outdated Show resolved Hide resolved
hooks/boot/01-kafo-hook-extensions.rb Outdated Show resolved Hide resolved
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this looks ok, but holding off until we merge tuning.

@@ -15,7 +15,7 @@ def error_exit(message, code)
kafo.class.exit code
end

if module_enabled?('katello')
if candlepin_enabled?
java_version_string = `/usr/bin/java -version 2>&1`
java_version = java_version_string.split("\n")[0].split('"')[1]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On an unrelated note: I think this check itself should ensure JDK 1.8 is used since 1.7 is too old.

katello/hooks/boot/13-hiera.rb Outdated Show resolved Hide resolved
@ekohl ekohl merged commit 71aee5b into theforeman:develop Aug 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants