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

Refs #30023: Enable Pulpcore RPM plugin #261

Merged
merged 1 commit into from
Jun 8, 2020

Conversation

ehelms
Copy link
Member

@ehelms ehelms commented Jun 4, 2020

No description provided.

@@ -363,6 +366,7 @@

include pulpcore::plugin::container
include pulpcore::plugin::file
include pulpcore::plugin::rpm
Copy link
Member

Choose a reason for hiding this comment

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

general question: shouldn't those plugin includes (at least file and rpm) be inside the respective "proxy to pulpcore" blocks? as otherwise we'd be installing the pulpcore plugin but not using it, which seems like a waste?

Copy link
Member

Choose a reason for hiding this comment

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

The idea is that the plugin is installed. This allows you to migrate content from Pulp 2. Also the Pulp 3 content URLs are always available. The "proxy to pulpcore" blocks are about routing the old Pulp 2 URLs to Pulp 3 so clients don't have to be modified. Ideally they should, but this allows to do so at a convenient pace.

@@ -287,7 +290,7 @@

class { 'pulp':
enable_ostree => $enable_ostree,
enable_rpm => $enable_yum,
enable_rpm => $enable_yum and !$proxy_pulp_yum_to_pulpcore,
Copy link
Member

Choose a reason for hiding this comment

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

Not really for this PR, but this made me think of it: we don't have conditional to disable crane if we proxy container content to Pulpcore.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea, I think we just were leaning towards leave Crane alone till we can safely rip it out.

Copy link
Contributor

@wbclark wbclark left a comment

Choose a reason for hiding this comment

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

LGTM

@wbclark wbclark merged commit 888c06b into theforeman:master Jun 8, 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.

5 participants