More controlled environment in the build #52
Conversation
andreaTP
force-pushed
the
build-improvements
branch
4 times, most recently
from
1e34226
to
fb574a0
Compare
|
Thanks to the help of @tarilabs we have verified that with those changes the build and the |
Signed-off-by: Andrea Peruffo <[email protected]>
andreaTP
force-pushed
the
build-improvements
branch
from
e2b3cc1
to
3548eab
Compare
There was a problem hiding this comment.
What I like about this PR:
- parametrize in bin/ the protoc toolchain, this was an issue with other contributors and seems this improves
- parametrize in bin/ the Go toolchain, as above
- avoid network mgt in docker-compose-local.yml
Things I haven't been able to confirm we need:
- buildvcs flag
- network: host settings/changes
per comments below.
Will add docker-compose.yml changes which I found beneficial when working with @andreaTP
Makefile
Outdated
| @@ -204,7 +212,7 @@ endif | |||
| # build docker image | |||
| .PHONY: image/build | |||
| image/build: | |||
| ${DOCKER} build . -f ${DOCKERFILE} -t ${IMG}:$(IMG_VERSION) | |||
| ${DOCKER} build . -f ${DOCKERFILE} -t ${IMG}:$(IMG_VERSION) --network host | |||
There was a problem hiding this comment.
tbh I'm not sure we need this change 🤔 , further it does not work with Podman desktop on my end:
There was a problem hiding this comment.
On podman Desktop on my Mac I get this error:
No routable interface for IPv6: IPv6 is disabled
Couldn't open network namespace /proc/4047/ns/net: Permission denied
docker-compose-local.yaml
Outdated
| @@ -13,8 +16,13 @@ services: | |||
| build: | |||
| context: . | |||
| dockerfile: Dockerfile | |||
| command: ["proxy", "--mlmd-hostname", "localhost", "--mlmd-port", "9090"] | |||
| network: host | |||
There was a problem hiding this comment.
I'm not sure we need this network: host 🤔
There was a problem hiding this comment.
it's used only during the build of the container and it was needed on my machine with podman to successfully build the image.
| @@ -13,8 +16,13 @@ services: | |||
| build: | |||
| context: . | |||
| dockerfile: Dockerfile | |||
| command: ["proxy", "--mlmd-hostname", "localhost", "--mlmd-port", "9090"] | |||
| network: host | |||
| command: ["proxy", "--hostname", "0.0.0.0", "--mlmd-hostname", "mlmd-server", "--mlmd-port", "8080"] | |||
There was a problem hiding this comment.
confirming "--hostname", "0.0.0.0", is needed 😅
| ${GO} build -buildvcs=false | ||
|
|
||
| .PHONY: build/odh | ||
| build/odh: vet | ||
| go build | ||
| ${GO} build -buildvcs=false |
There was a problem hiding this comment.
Was -buildvcs=false needed only for the build-in-docker? 🤔 Because on my end the make build works without issue as-is, without needing -buildvcs=false
There was a problem hiding this comment.
it fails in CI without this flag ...
Signed-off-by: Matteo Mortari <[email protected]>
|
@dhirajsb: changing LGTM is restricted to collaborators In 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. |
Using host network is not a "best practice" and with those changes, we are improving it.
The DB should be exposed on the docker compose network too, should we modify any additional files? |
|
@andreaTP looks like the default store config file is using an in-memory SQLite DB, so the network is not an issue. |
|
@dhirajsb: changing LGTM is restricted to collaborators In 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. |
Signed-off-by: Matteo Mortari <[email protected]>
|
With e50cd37 I've added support for protoc in ARM and avoiding the use of network-host as per comment. Tested with:
based on OP and review, will eventually merge this one. |
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andreaTP, dhirajsb, lampajr, tarilabs The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
* Update(openshift-ci/scripts): Increase API version of Model Registry * Update(test/scripts): Increase API version of Model Registry
Improvements to make the build self-contained.
Description
Various minor improvements to make sure that we are using the same versions of the tools and more.
How Has This Been Tested?
Building locally on Mac and building the container image with
podmanMerge criteria: