-
-
Notifications
You must be signed in to change notification settings - Fork 63
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
fix docker build problem and added arm64 support #67
base: beta
Are you sure you want to change the base?
Conversation
fixed docker build problems on github actions everything is tested and working as expected
What problem are you addressing? |
|
You didn't add the arch that I removed in that commit due to error though. Arm64 is currently working; arm64/v8 is the arch I tried to add that threw an error. |
check now |
Which specific change fixed the error that was being thrown for the linux/amd64/v8 arch before? |
@@ -1,11 +1,25 @@ | |||
FROM python:3.11-slim-buster | |||
FROM python:slim |
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.
Why? I like to pin the python version, so it won't automatically start using v12 without me testing it. I'm not too concerned about slim vs. slim-buster, but what is your reasoning for slim over slim-buster? It appeared slim-buster supported more architectures when I was looking before.
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.
slim uses debian bullseye and buster uses debian buster
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.
Why do you think slim should be used over slim-buster? And please update to pin python 3.11.
LABEL org.opencontainers.image.description="Docker for SEARCHARR" | ||
LABEL Name=Searcharr Version="v1.2-beta" | ||
|
||
ARG TARGETPLATFORM BUILDPLATFORM |
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.
What is the purpose of these? I don't see the args being used anywhere.
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.
ARG TARGETPLATFORM BUILDPLATFORM
is a must for multi architecture builds
and using labels are not necessary but for tagging name and dockerhub repository to this github repository it is used...
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.
Are the ARGs what was stopping the arm64/v8 arch build from working? Why do the other archs work without these ARGs?
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.
Please also remove -beta
from the Dockerfile version and increment to v1.3
RUN chmod -R 777 /app && \ | ||
chmod -R +x /app && \ | ||
chmod -R 705 /app |
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.
Why?
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.
gives full permissions to the /app directory in case of any Permissions Denied
errors
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.
Did you encounter any Permission Denied errors? I have not, and I have not received any reports of such issues. This seems unnecessary.
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.
Comments added inline
i have fixed them now @toddrob99 😁 |
@@ -2,7 +2,7 @@ version: '3.0' | |||
services: | |||
searcharr: | |||
container_name: searcharr | |||
image: toddrob/searcharr:latest | |||
build: . |
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.
Why build instead of pulling the image? That will require that users clone the repo instead of just creating a docker-compose.yml file.
push: true | ||
tags: toddrob/searcharr:beta | ||
platforms: linux/amd64,linux/arm64,linux/arm/v7,linux/arm64/v8 | ||
tags: toddrob/searcharr:latest |
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 is the beta.yml workflow file, so the docker tag needs to be toddrob/searcharr:beta
.
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 appears to still be incorrect
Thanks @AmirulAndalib. Please note there are several other comment threads listed here. I want to make sure you saw all of them and not just the last one. |
those are enhancements |
fixed docker build problems on github actions everything is tested and working as expected