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 #30436 - add import-export paths to katello answers #615

Closed

Conversation

jeremylenz
Copy link

@jeremylenz jeremylenz commented Nov 17, 2020

This is the third PR from theforeman/puppet-pulpcore#147 (review)

  • override the default $allowed_{import,export}_path parameters for the Katello scenario only
  • define a migration for the Katello scenario that sets the proper values of the parameters for users who already installed

(thanks to @chris1984 for some guidance)

config/katello-answers.yaml Outdated Show resolved Hide resolved
@@ -0,0 +1,4 @@
if answers['foreman_proxy_content'].is_a?(Hash)
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we need to account for a possible case where answers['foreman_proxy_content'] == true rather than being a hash. I think some other migration would still run first and convert it to a hash but I'm not 100% positive.

Copy link
Author

Choose a reason for hiding this comment

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

All the other migrations I see use the same check, if answers['foreman_proxy_content'].is_a?(Hash). The only exception is config/katello.migrations/200123161606-enable-pulpcore.rb:

  unless answers['foreman_proxy_content']
    answers['foreman_proxy_content'] = {} unless answers['foreman_proxy_content'].is_a?(Hash)
    answers['foreman_proxy_content']['proxy_pulp_isos_to_pulpcore'] = false
  end

This block would only run if it were false or nil, and would leave it unchanged in the event it were true.

Using answers['foreman_proxy_content'].is_a?(Hash) as a check seems reasonable to me, since it's also used by so many other migrations and I assume those don't break anything.

@ekohl
Copy link
Member

ekohl commented Nov 20, 2020

FYI: if the defaults are correct you don't need a migration. So I think that in this case where they are new and in a very deployment specific scenario. I doubt you really need this PR at all and rather get them right in the puppet-foreman_proxy_content PR.

@wbclark
Copy link
Contributor

wbclark commented Nov 21, 2020

FYI: if the defaults are correct you don't need a migration. So I think that in this case where they are new and in a very deployment specific scenario. I doubt you really need this PR at all and rather get them right in the puppet-foreman_proxy_content PR.

Katello and FPC scenarios should get different values, because FPC has no need for CV import/export.

So if we set correct defaults for FPC in puppet-FPC, then we need a migration in the Katello scenario to support CV import/export.

Co-authored-by: William Bradford Clark <wclark@redhat.com>
@jeremylenz
Copy link
Author

[test foreman-installer]

not sure how to address the testing situation here

@jeremylenz
Copy link
Author

jeremylenz commented Dec 3, 2020

@ekohl @ehelms In light of the changes in theforeman/puppet-foreman_proxy_content#297 and the comment above, should this PR be closed?

@ekohl
Copy link
Member

ekohl commented Dec 3, 2020

Yes

@ekohl ekohl closed this Dec 3, 2020
@jeremylenz jeremylenz deleted the 30436-allowed-exports branch December 3, 2020 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants