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 #29184 - Add cdn-ssl param migration hook #477

Merged
merged 1 commit into from
Mar 30, 2020
Merged

Fixes #29184 - Add cdn-ssl param migration hook #477

merged 1 commit into from
Mar 30, 2020

Conversation

chris1984
Copy link
Member

@chris1984 chris1984 commented Feb 26, 2020

No description provided.

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.

Please have a look at the whitespace.

katello/hooks/pre/32-cdn_setting.rb Outdated Show resolved Hide resolved
@chris1984
Copy link
Member Author

chris1984 commented Feb 26, 2020

@ekohl updated to fix the rubocop issues.

I did part of it in the pre hook since I noticed when doing everything in the post hook, we did not remove the parameter in the /etc/foreman/plugins/katello.yaml file.

When I ran the installer again to confirm that the logic was working correctly, I noticed it removing the param from the katello.yaml file. I can do everything in the post hook but I think I would need to use sed to remove the entry from the katello.yaml file unless there is a better way?

@ekohl
Copy link
Member

ekohl commented Feb 26, 2020

I did part of it in the prehook since I noticed when doing everything in the post hook, we did not remove the parameter in the /etc/foreman/plugins/katello.yaml file.

That's a good point and indeed needed. I was only thinking about the foreman-rake config part and forgot the rest.

@chris1984
Copy link
Member Author

@ehelms @ekohl updating and working :) I tested with the puppet-katello pr and here is the results of my testing:

Pre Commit Hook:

[ INFO 2020-03-19T13:39:22 verbose] Executing hooks in group pre_commit
[ INFO 2020-03-19T13:39:28 verbose] cdn_ssl_version param found, storing for post hook

Removing it from katello.yaml file since puppet code is gone:

[ WARN 2020-03-19T13:41:37 verbose]  /Stage[main]/Katello::Application/File[/etc/foreman/plugins/katello.yaml]/content: 
[ WARN 2020-03-19T13:41:37 verbose] --- /etc/foreman/plugins/katello.yaml	2020-03-18 17:55:15.435217825 +0000
[ WARN 2020-03-19T13:41:37 verbose] +++ /tmp/puppet-file20200319-5570-ay4sq4	2020-03-19 13:41:37.581697239 +0000
[ WARN 2020-03-19T13:41:37 verbose] @@ -2,7 +2,6 @@
[ WARN 2020-03-19T13:41:37 verbose]  ## Module: puppet-katello
[ WARN 2020-03-19T13:41:37 verbose]  
[ WARN 2020-03-19T13:41:37 verbose]  :katello:
[ WARN 2020-03-19T13:41:37 verbose] -  :cdn_ssl_version: SSLv23
[ WARN 2020-03-19T13:41:37 verbose]    :rest_client_timeout: 3600
[ WARN 2020-03-19T13:41:37 verbose]  
[ WARN 2020-03-19T13:41:37 verbose]    :content_types:
[ INFO 2020-03-19T13:41:37 verbose]  Computing checksum on file /etc/foreman/plugins/katello.yaml
[ INFO 2020-03-19T13:41:37 verbose]  /Stage[main]/Katello::Application/File[/etc/foreman/plugins/katello.yaml]: Filebucketed /etc/foreman/plugins/katello.yaml to puppet with sum 1f74ed4f03f430a2574a4cca5f4e3feb
[ WARN 2020-03-19T13:41:37 verbose]  /Stage[main]/Katello::Application/File[/etc/foreman/plugins/katello.yaml]/content: content changed '{md5}1f74ed4f03f430a2574a4cca5f4e3feb' to '{md5}778fa03cb7efcc2f6e5752b558077bbb'

Post Hook Running:

[ INFO 2020-03-19T13:46:32 verbose] cdn_ssl_version param found, migrating to a Katello setting
[ INFO 2020-03-19T13:47:03 verbose] All hooks in group post finished

Screenshot showing it in the settings page:

https://nimbusweb.me/s/share/3985742/gy42ph4587lctbb1dw92

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.

You need to clear the value for cdn_ssl_version in the custom config to avoid calling the same rake task every time.

katello/hooks/post/31-cdn_setting.rb Outdated Show resolved Hide resolved
@chris1984
Copy link
Member Author

@ekohl how does this look, if it looks good ill test it again.

katello/hooks/post/31-cdn_setting.rb Outdated Show resolved Hide resolved
katello/hooks/pre_commit/14-cdn_setting.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 correct though I didn't test it.

katello/hooks/post/31-cdn_setting.rb Outdated Show resolved Hide resolved
katello/hooks/pre_commit/14-cdn_setting.rb Outdated Show resolved Hide resolved
@chris1984
Copy link
Member Author

chris1984 commented Mar 19, 2020

@ekohl updated and it works again with testing, only difference this time with using execute during the post hook we are being transparent to the user what we are doing which looks better:

[ INFO 2020-03-19T19:48:49 verbose] cdn_ssl_version param found, migrating to a Katello setting
cdn_ssl_version: SSLv23
foreman-rake -- config -k cdn_ssl_version -v 'SSLv23' finished successfully!
[ INFO 2020-03-19T19:49:15 verbose] All hooks in group post finished

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.

I'm debating whether it should be pre_validations rather than pre_commit. The values should already be present then and there's less code that could interfere with it (no validations nor the entire interactive mode). However, I have insufficient experience with hooks to know whether it can really be a problem. @ehelms any thoughts?

katello/hooks/post/31-cdn_setting.rb Outdated Show resolved Hide resolved
katello/hooks/pre_commit/14-cdn_setting.rb Outdated Show resolved Hide resolved
katello/hooks/pre_commit/14-cdn_setting.rb Outdated Show resolved Hide resolved
@chris1984
Copy link
Member Author

@ekohl @ehelms updated with the comment and requested changes.

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.

@ehelms any last comments?

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.

4 participants