-
Notifications
You must be signed in to change notification settings - Fork 36
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
Remove Pulp 2 functionality #271
Conversation
manifests/pub_dir.pp
Outdated
additional_includes => ["${apache::confd_dir}/pulp-vhosts80/*.conf"], | ||
custom_fragment => template('foreman_proxy_content/httpd_pub.erb'), | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With Pulp 2 removed, we still need the pub dir (but not necessarily this implementation of it). This is responsible for some non-Pulp related things:
- serving katello-consumer-rpm
- serving CA cert
- serving katello-client-bootstrap
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohh, I see you moved that up into init
I am seeing the following testing this with the pulpcore change:
|
There is also a failure in creating the vhost fragments for Pulpcore, looks like the vhost priority needs to be passed in if attaching to another vhost.
|
I have not even tried to run it myself. It was only me quickly going over the code and see what would roughly be the steps and things that should remain.
I suspected as much, see the TODO there. Perhaps it's better to make it part of the SELinux policy. Same for Foreman itself. theforeman/puppet-foreman#849 is another example of someone who ran into this.
Looks like Foreman uses the |
I don't expect this to fully work yet :) But there is enough work that needs this that I am helpin speed up the testing.
From testing this, it's available to be retrieved. We should need to pass it in here -- https://github.com/theforeman/puppet-foreman_proxy_content/pull/271/files#diff-60ae41fd0a31977447947f59940ee9a4R125 as something like |
manifests/init.pp
Outdated
Stdlib::Port $reverse_proxy_port = 8443, | ||
|
||
Optional[String] $rhsm_hostname = undef, | ||
String $rhsm_url = '/rhsm', # TODO: Not really a URL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably shouldn't be configurable to be honest, the API is the API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having it live in a variable is generally a good thing and I think this still has some room to split functionality in smaller classes. Then you can make it a parameter on the private class API but not expose it in the public class API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm good with it being a variable, I meant the perceived intent that a user can and should change the value is wrong. This is a hard-coded API in the Katello plugin so having this be changeable by someone using the public interface of this module can result in incorrect behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, but I'm first going to focus on the Pulp parts. It should probably be done as part of #272 since that does introduce the private API I was thinking about.
Updated:
|
Could you update this next time around to point to https://projects.theforeman.org/issues/30359 |
This is handled in the spec helper and only leads to a duplicate report.
When Pulp 2 parameters are set, Pulp 3 parameters should also be set. This disabled Pulp 3 in many cases leading to faster spec tests.
With RSpec contexts can be nested and they'll inherit the parent let blocks. The descriptions will be included as well.
Rebased just to see how it would look. This still assumes Pulp 2 and qpid are removed at the same time. |
Closing in favor of #306 |
Includes #268
Various TODOs in the code that should be looked at.