-
Notifications
You must be signed in to change notification settings - Fork 115
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
Refactor ipv6 http_proxy code #16928
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
HTTP_PROXY: | ||
UN_AUTH_PROXY_URL: # http://proxy-01.example.com:3423 | ||
HTTP_PROXY_IPV6_URL: # http://proxy-01.ipv6.example.com:3423 | ||
AUTH_PROXY_URL: # http://proxy-02.example.com:3423 | ||
USERNAME: auth-proxy-user | ||
PASSWORD: auth-proxy-password |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,12 +36,6 @@ | |
Validator('server.ssh_password', default=None), | ||
Validator('server.verify_ca', default=False), | ||
Validator('server.is_ipv6', is_type_of=bool, default=False), | ||
# validate http_proxy_ipv6_url only if is_ipv6 is True | ||
Validator( | ||
'server.http_proxy_ipv6_url', | ||
is_type_of=str, | ||
when=Validator('server.is_ipv6', eq=True), | ||
), | ||
], | ||
content_host=[ | ||
Validator('content_host.default_rhel_version', must_exist=True), | ||
|
@@ -161,6 +155,12 @@ | |
'http_proxy.password', | ||
must_exist=True, | ||
), | ||
# validate http_proxy_ipv6_url only if server.is_ipv6 is True | ||
Validator( | ||
'http_proxy.http_proxy_ipv6_url', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think keeping the global HTTP Proxy for all satellite applications, the server conf is a good place. HTTP Proxy conf is made to configure within the satellite, whereas IPV6 HTTP proxy is inside and outside as well to have cross network communication. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Another reason keeping it in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. By that logic, we'll be making server.yaml unnecessarily bulky and a lot of other settings could fall into such reasoning. Not to mention that we'll be basically ignoring the very general rule of clustering similar settings in same yaml. But I'd leave the final decision to you/JPL. I can move it back to |
||
is_type_of=str, | ||
when=Validator('server.is_ipv6', eq=True), | ||
), | ||
], | ||
ipa=[ | ||
Validator( | ||
|
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.
Arent we duplicating the
sat
here?The object name
sat
itself prouds the reference onto which HTTP proxy is being enabled. Hence the explicitsatellite
keyword in function name should now be needed.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 don't understand what you mean; your code suggestion has no change.
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.
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.
Isn't it better to be explicit in the name? This method does setup related to Satellite - it changes Satellite's settings through API.
I, as a code reader, find it more straightforward to read
enable_satellite_ipv6_http_proxy
orenable_sat_ipv6_http_proxy
as it tells me that it does something with Satellite itself.The
enable_ipv6_http_proxy
method can be defined in theContentHost
class to do something completely different from modifying Satellite settings.Host object methods count is constantly increasing, therefore I advocate for explicitness - for keeping
satellite
orsat
in the method name.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 agree that
satellite
is redundant here, the vast majority of the functionality in this repo acts against a Satellite and this method is defined under a Satellite class.how about we really clarify what's going on with the name by changing the name to reflect that this is updating proxy settings to use an ipv6 proxy.
perhaps something like
ensure_ipv6_proxy_settings