-
Notifications
You must be signed in to change notification settings - Fork 60
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
Makefile: scripts: Add build args for proxy when using docker build #385
Makefile: scripts: Add build args for proxy when using docker build #385
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.
lgtm, thanks @GabyCT!
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.
The code LGTM, but one question about where the http_proxy
and https_proxy
values are set - is the expectation that these environment variables are set on the self-hosted runner independently of the operator workflow?
It looks like we get unbound variable errors when http_proxy isn't set, so I guess we need to make that behaviour safe too.
c8d6fac
to
50a84cf
Compare
@stevenhorsman changes applied |
@@ -143,9 +146,9 @@ run: manifests generate fmt vet ## Run a controller from your host. | |||
docker-build: test ## Build docker image with the manager. | |||
ifneq (, $(PEERPODS)) | |||
@echo PEERPODS is enabled | |||
docker build -t ${IMG} -f Dockerfile.peerpods . | |||
docker build --build-arg http_proxy=$(http_proxy) --build-arg https_proxy=$(https_proxy) -t ${IMG} -f Dockerfile.peerpods . |
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.
is there a reason why this approach was chosen over the better alternative which just setting the ~/.docker/config.json
?
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.
@mythi I have the proxies at the config but seems that when building the dockerfile is still failing without those arguments
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 prefer this approach. It'd be good to understand why the best option does not work
550c956
to
dec86b7
Compare
This PR adds build args for proxy when using docker build, this is specially needed when we are behind a proxy to avoid failures. Signed-off-by: Gabriela Cervantes <[email protected]>
@stevenhorsman now the CI for tdx is passing |
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.
Thanks @GabyCT
This PR adds build args for proxy when using docker build, this is specially needed when we are behind a proxy to avoid failures.