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.

Sign up for GitHub

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

Jump to bottom

Fixes #28476 - Add option for CDN SSL Versions to Settings #8552

Merged
merged 1 commit into from
Apr 6, 2020
Merged

Fixes #28476 - Add option for CDN SSL Versions to Settings #8552

merged 1 commit into from
Apr 6, 2020

Conversation

chris1984
Copy link
Member

No description provided.

@theforeman-bot
Copy link

Issues: #28476

@chris1984 chris1984 mentioned this pull request Jan 31, 2020
Merged
ekohl
ekohl reviewed Jan 31, 2020
View reviewed 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.

How do we handle the migration? Should puppet-katello start setting the setting (if present) and stop writing it to the YAML config? Should the installer completely stop managing this setting and leave it to the user to set it again if they need it?

@chris1984
Copy link
Member Author

@ekohl Would you be fine with a rake task that reads the yaml file and if there is an option then we update the setting in Katello? If it was just in the installer moving around I would propose a migration there but a rake task seems appropriate, thoughts?

@ekohl
Copy link
Member

ekohl commented Jan 31, 2020

An alternative is to use a foreman-installer hook. If the answer is present, we can call foreman-rake config -k cdn_ssl_version -v #{answers['katello']['cdn_ssl_version']} after running the installer and then drop the answer.

For the direct users of puppet-katello we can add a note in the release notes but I doubt they use this setting.

@chris1984
Copy link
Member Author

An alternative is to use a foreman-installer hook. If the answer is present, we can call foreman-rake config -k cdn_ssl_version -v #{answers['katello']['cdn_ssl_version']} after running the installer and then drop the answer.

For the direct users of puppet-katello we can add a note in the release notes but I doubt they use this setting.

The hook route seems the easiest and less error prone. I will make a hook then, I just know hooks are generally looked down upon. I will make a pr to the installer with a refs for you to review.

@ekohl
Copy link
Member

ekohl commented Jan 31, 2020

I just know hooks are generally looked down upon

Not entirely true, but hooks should only be used when they make sense. I think this is one of those cases.

@chris1984
Copy link
Member Author

@ekohl I think once this is merged theforeman/puppet-katello#320 I think this one is good then? The installer one was merged this morning.

ekohl
ekohl approved these changes Apr 1, 2020
View reviewed 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.

I can't speak for the actual code so I'll leave that up to the katello devs but 👍 for it. We already merged the installer side.

@chris1984 chris1984 requested a review from jturel April 1, 2020 13:45
jturel
jturel suggested changes Apr 1, 2020
View reviewed changes
app/models/setting/content.rb Show resolved Hide resolved
app/lib/katello/resources/cdn.rb Outdated Show resolved Hide resolved
app/models/setting/content.rb Outdated Show resolved Hide resolved
test/models/setting_test.rb Outdated Show resolved Hide resolved
@chris1984
Copy link
Member Author

@jturel updated! Screenshot of new setting change:

https://nimbusweb.me/s/share/4045678/fz6dtewbym4awma8swg1

jturel
jturel suggested changes Apr 2, 2020
View reviewed changes
Copy link
Member

@jturel jturel left a comment

Choose a reason for hiding this comment

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

Looks great, two small requests!

app/lib/katello/resources/cdn.rb Outdated Show resolved Hide resolved
test/lib/resources/cdn_test.rb Show resolved Hide resolved
@chris1984
Copy link
Member Author

@jturel I like to keep it interesting ;) copy and paste fail. Updated

jturel
jturel approved these changes Apr 3, 2020
View reviewed changes
Copy link
Member

@jturel jturel left a comment

Choose a reason for hiding this comment

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

APJ

@chris1984 chris1984 merged commit f300710 into master Apr 6, 2020
@chris1984 chris1984 deleted the ssl-option branch April 6, 2020 12:44
@wbclark wbclark mentioned this pull request Jan 23, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ekohl ekohl ekohl approved these changes

@jturel jturel jturel approved these changes

Assignees
No one assigned
Labels
Needs testing Not yet reviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@chris1984 @theforeman-bot @ekohl @jturel