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

Update Pulp plugin to drop Pulp 2 #638

Merged
merged 1 commit into from
Apr 9, 2021
Merged

Conversation

ehelms
Copy link
Member

@ehelms ehelms commented Dec 16, 2020

Katello 4.0 drops Pulp 2 in favor of Pulp 3 completely. If this
plugin is used, then Pulpcore will be used and the user can decide
if it should be deployed as a mirror configuration or not. This drops
most of the parameters as they are not relevant with Pulp 3.

I was not sure if disabling the Pulp 2 plugins was all that needed doing. At some point we will want to clean them up but figured I'd start by just disabling them. This is obviously backwards incompatible. I prefer to have latest reflect the state of Katello 4.0 given its so different. We could resort to erroring out when a user attempts to use parameters that dont work with 4.0, but that also means we have to keep around a lot this parameter cruft for a lot longer period of time. Further, in the installers use case, this would present misleading parameters.

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 don't like it when the software underneath still supports all the functionality. IMHO we should first update the plugin and then update this module to clean out the unused files.

@ehelms
Copy link
Member Author

ehelms commented Dec 16, 2020

If we update the plugin first, then we have to, in a coordinated fashion, release a new version of it and update the configuration management layer to prevent breakages. If we update the config management layer to reflect the state we want to be in, then we can more easily remove the old functionality out of the plugin. I am happy to file an issue to track against the plugin for it to get updated, but that should not be a blocker to how we are deploying. Just because some piece of underlying software supports something, does not mean we have to choose to expose it to the user.

@ekohl
Copy link
Member

ekohl commented Dec 17, 2020

theforeman/smart_proxy_pulp#26 would be the update that drops Pulp 2 on the plugin side.

@ehelms
Copy link
Member Author

ehelms commented Dec 17, 2020

I still stand by we can avoid chicken-and-egg, fire off and see if nightly works orchestration, by making this update, turning those plugins off and then having all the code cleaned up. Just because the plugin software supports does not mean our deployment needs to (or should).

@ekohl
Copy link
Member

ekohl commented Dec 17, 2020

I just have a fundamental issue with this. Essentially this is an anti-feature. It's backwards incompatible anyway, so why not go the whole way and really drop it? I don't want to merge this patch so don't expect an ACK from me.

@ehelms
Copy link
Member Author

ehelms commented Dec 17, 2020

I don't mind just dropping it I assume that means removing (https://github.com/theforeman/puppet-foreman_proxy/pull/638/files#diff-8fb2d14548a55c08f905d8327857148fe6c08d960ce0a42703cd99eba72dc2ebR33-R44) and that I would need to add lines to ensure that the configuration files are absent for a release since as far as I can tell nothing cleans those up.

@ekohl
Copy link
Member

ekohl commented Dec 17, 2020

Correct. See foreman_proxy::settings_file { 'pulp3': as an example. This is a side effect of us renaming pulp3 to pulpcore a while back.

@ehelms
Copy link
Member Author

ehelms commented Dec 18, 2020

Updated to drop the pulp and pulpnode plugins all together and ensure their settings files are removed.

manifests/plugin/pulp.pp Outdated Show resolved Hide resolved
spec/classes/foreman_proxy__plugin__pulp_spec.rb Outdated Show resolved Hide resolved
spec/classes/foreman_proxy__plugin__pulp_spec.rb Outdated Show resolved Hide resolved
spec/classes/foreman_proxy__plugin__pulp_spec.rb Outdated Show resolved Hide resolved
templates/plugin/pulp.yml.erb Outdated Show resolved Hide resolved
templates/plugin/pulpnode.yml.erb 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.

Looks good to me. Let's get a 3.0 release out and packaged in nightly, then merge this.

Katello 4.0 drops Pulp 2 in favor of Pulp 3 completely. If this
plugin is used, then Pulpcore will be used and the user can decide
if it should be deployed as a mirror configuration or not. This drops
most of the parameters as they are not relevant with Pulp 3.
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