-
Notifications
You must be signed in to change notification settings - Fork 26
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
CI: build an additional arm64
docker image
#110
Conversation
Hi @Tomaszal, thanks, with @vascoguita we have reviewed this and we rather propose to use |
|
Use python:3.10-slim-buster for arm64 and amd64 docker images
Done, also switched apk to apt package manager, as that's what the base image uses |
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.
Looks good to me, @micbar do you see any major drawback in using python:3.10-slim-buster
as opposed to the classic alpine
also for the usual amd64 build?
@glpatcern This is bad from a maintainablility POV I wanted to avoid a See my trivy scan report
This will pop up on every customer security scan in the future. In my opinion, we should not "call for trouble" if we do not need to. Can we find something better maintained? |
All right, understood. Let's cook something tailored for arm64. The original PR indeed had all separated, but with quite some duplication. I see we can at least parameterize the Dockerfile. |
I've seen that the job failed on the master branch after the latest commit so I checked the logs. It seems the build arguments aren't properly set. Currently they are comma separated ( |
@Tomaszal good catch, we fixed the build arguments in master (and previously I merged this PR to master to ease the debugging process, though it's not yet complete). Now the build complains because presumably For reference: https://github.com/cs3org/wopiserver/actions/runs/4142747553 |
@glpatcern this if statement doesn't seem to work, as it's still trying to run |
Interestingly we tried several variants of that |
@glpatcern I see, I've tried playing around with it a bit and wasn't able to figure out why it wasn't expanding the variable correctly. However, I did come up with an alternative that should work just fine as well. Replace this if statement: RUN if [ '$BASEIMAGE' = 'python:3.10-slim-buster' ]; then \
apt -y install g++; \
else \
apk add g++; \
fi With this: RUN if command -v apt &> /dev/null; then \
apt -y install g++; \
elif command -v apk &> /dev/null; then \
apk add g++; \
fi This is probably a better solution anyway, as it doesn't require hard coding the base image name into the if statement. |
Good point, thanks for this! I've pushed the change (including an extra |
Resolves #109.
Blocked by cs3org/reva#3642.
I've never used GitHub workflows before, so I'm not 100% sure this will work as expected.
Also, I created a separate
Dockerfile
forarm64
as I had to switch frompython:3.11-alpine
topython:3.10-slim-buster
base image. This is because it was failing to building onalpine
even with the changes mentioned on grpc/grpc#24722, and grpcio failed to build onpython-3.11
even onslim-buster
.