-
Notifications
You must be signed in to change notification settings - Fork 202
Allow installer to use a proxy #1341
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
Allow installer to use a proxy #1341
Conversation
How do we expect developers to deploy the proxy for local testing? I was expecting to still need something similar to #1192 so we can easily enable a proxy on the virthost, even if that's not required in CI? |
08a069c to
118543f
Compare
I think we could add another block to kick off the squid container, using something similar to CI test. The immediate goal anyhow now was to support the CI. |
118543f to
8b54de4
Compare
update: waiting for the job results, but very likely the already existing proxy defined by the CI cannot be used as it is, so another one should be required |
8b54de4 to
9d9c4be
Compare
|
Revamping #1192 |
9d9c4be to
35b929f
Compare
35b929f to
2625548
Compare
|
/test pj-rehearse |
|
@andfasano: The specified target(s) for
The following commands are available to trigger optional jobs:
Use
DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/retest |
|
/test e2e-metal-ipi |
2625548 to
c756ded
Compare
c756ded to
357de2a
Compare
|
Added ipv6 support /hold |
357de2a to
612ca4a
Compare
|
/hold cancel @hardys @derekhiggins The patch looks ready, could you please review it? Thanks |
6829daa to
6e2a4fa
Compare
001ebc9 to
21f2ee7
Compare
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hardys The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
| # Proxy related configuration | ||
| if [[ ! -z "$INSTALLER_PROXY" ]]; then | ||
| export EXT_SUBNET=${EXTERNAL_SUBNET_V6} | ||
| if [[ "$IP_STACK" = "v4" ]]; then |
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.
nit: double = for consistency
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 used the same approach for the preovious ip_stack comparisons in the same file, maybe we could also address such kind of more global changes in some other prs as also @hardys suggested
| fi | ||
|
|
||
| # Configure a local proxy to be used for the installation | ||
| if [[ ! -z "${INSTALLER_PROXY}" ]]; then |
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.
nit: -n instead of ! -z
guess it's too late as we use ! -z everywhere!
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.
Yeah we should perhaps consider a future PR to align on one convention (and other issues like quoting and [ vs [[ - not a blocker for this though IMO since it's a cosmetic (and as you say pre-existing) issue
|
/lgtm |
|
/test e2e-metal-ipi-serial-ovn-ipv6 |
2 similar comments
|
/test e2e-metal-ipi-serial-ovn-ipv6 |
|
/test e2e-metal-ipi-serial-ovn-ipv6 |
|
/test e2e-metal-ipi-serial-ovn-ipv6 |
|
/test e2e-metal-ipi-serial-ovn-ipv6 |
Looking at sippy this seems to still be perma-failing, seems like we need the https://bugzilla.redhat.com/show_bug.cgi?id=2049117 pending timeout fixes? If you're happy that's the cause I'd suggest we override this job to make progress. |
|
/test e2e-metal-ipi-ovn-dualstack |
Yes, I've also prepared a PR (openshift/origin#26833) to be used for fixing it (by @bfournie openshift/origin#26810), we can override it |
|
/override ci/prow/e2e-metal-ipi-serial-ovn-ipv6 |
|
@andfasano: /override requires a failed status context or a job name to operate on.
Only the following contexts were expected:
DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/override ci/prow/e2e-metal-ipi-serial-ovn-ipv6 |
|
@andfasano: Overrode contexts on behalf of andfasano: ci/prow/e2e-metal-ipi-serial-ovn-ipv6 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/override ci/prow/e2e-metal-ipi-serial-ovn-ipv6 |
|
@hardys: Overrode contexts on behalf of hardys: ci/prow/e2e-metal-ipi-serial-ovn-ipv6 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/test e2e-metal-ipi |
|
/override ci/prow/e2e-metal-ipi-serial-ovn-ipv6 |
|
@andfasano: Overrode contexts on behalf of andfasano: ci/prow/e2e-metal-ipi-serial-ovn-ipv6 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
This patch adds a new config var
INSTALLER_PROXYso that the installer could use a local proxy (in our CI jobs, we already deploy a proxy in the baremetalds-devscript-proxy step). In addition, all outgoing traffic will be disabledCo-author: @derekhiggins
cc @hardys