-
Notifications
You must be signed in to change notification settings - Fork 3
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
feat: add support for http_proxy #175
Conversation
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.
Thank you @shipperizer for the contribution.
In addition to my comments below, can you please also improve the PR description so that reviewers know why this is needed. Please also specify in there that this PR fixes #125
@@ -224,7 +224,7 @@ def _push_csr_to_workload(self, csr: str) -> None: | |||
def _execute_lego_cmd(self) -> bool: | |||
"""Execute lego command in workload container.""" | |||
process = self._container.exec( | |||
self._cmd, timeout=300, working_dir="/tmp", environment=self._plugin_config | |||
self._cmd, timeout=300, working_dir="/tmp", environment=self._common_config | self._plugin_config |
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.
This lib is owned by the lego k8s project. Let's make the change over there, bump the lib version, then pull it here.
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.
http_proxy: | ||
description: URL of the HTTP proxy eg http://proxy.internal:6666, it will set the HTTP_PROXY var in the workload environment | ||
type: string | ||
default: '' | ||
https_proxy: | ||
description: URL of the HTTPS proxy eg http://proxy.internal:6666, it will set the HTTPS_PROXY var in the workload environment | ||
type: string | ||
default: '' | ||
no_proxy: | ||
description: Domains that need to be excluded from proxying no_proxy="test.com,test.co.uk", it is a comma separate list | ||
type: string | ||
default: '' |
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.
Just like it is done in GitHub Runners charm and other IS charms, let's use the Juju env variables for this.
Reference
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 happy to change it
although using those env variables might be counterproductive
ideally you have 2 layers, one where u use those by default and another where u can override that behaviour
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 placed the same comment on the lego k8s lib, let's discuss it over there)
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 have a strong opinion, but I see there was a discussion here. And the point was made to keep consistency across charms.
In addition using the juju model env vars would allow us to handle this in the lib only without the need to change any of the Lego charms.
This has been addressed by #178 and canonical/lego-base-k8s-operator#143 |
No description provided.