-
Notifications
You must be signed in to change notification settings - Fork 27
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
Dockerfile lint #689
Dockerfile lint #689
Conversation
* DL3008 pin apt-get packages * DL3013 and SC2261 pin pip versions * DL3059 consecutively RUN commands * DL3042 no cache on pip install * DL3003 use of WORKDIR instead of cd * DL3045 use WORKDIR using COPY * DL3009 delete apt-get lists * DL3015 avoid recommended packages
* DL3045 set WORKDIR before COPY * DL3003 use WORKDIR instead of cd * DL3042 no cache dir for pip * DL3015 no recommended packages on apt installation * DL3042 no cache dir for pip * DL3059 multiple RUN consecutively * DL3008 pin apt packages version * SC3014 use = instead of == * SC2086 use double quote * DL3006 the arch must be variable
@Tansito FYI: Changed Dockerfiles build images fine on my M1 Mac. |
Thank you @akihikokuroda ! I was going to ask you for that. I'm try to fix the errors that I couldn't test in my local. I will let you know when I have it |
This reverts commit 8fb9ce2.
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.
🕺
@akihikokuroda or @pacomf if you can check now that I finished that I didn't miss anything related with ARM chips it would be awesome 🙏 |
The ray-node image build is failing.
|
@akihikokuroda out of curiosity, do the image builds work if you run docker via colima? |
@Tansito If I take out the version from these 2 lines. It builds at least. I haven't run using them yet.
|
@psschwei The docker build of the ray-node fails via colima, too. Taking out version from these lines fix the issue via colma, too. It seems some package version conflicts of arm64 packages. |
🤔 what arch is colima using? |
@psschwei it is building the image for arm64. The CI build is building building the images for both amd64 and arm64. If it can not build the image for arm64, it fails. |
I get that. But if colima would let m1 macs run x86 images locally, then maybe we don't have to worry about multi-architecture builds anymore |
I'm not sure if we can say that colima is the only environment we support for M1 Mac. I think that the multi-arch build should be our direction. |
My two cents: the end game is to run this on Kubernetes, which at least for IBM Cloud has to be x64. So while arm64 images are nice for local dev, are they going to be run in production? If not, and we have a way to run x86 on m1s, then it seems like a lot of effort to be maintaining two builds... That said, I'm on an x86 machine, so I'm more than a little biased here 😄 Curious what @pacomf thinks here. |
My two cents 😄 Some enterprise user may want to run this on their Power or Z System. So it's better for us to have the framework for it. |
Agree with @psschwei and @akihikokuroda hahaha. Right now our goal is the @psschwei thoughts... but we need to be sure that we cant lose the focus about the @akihikokuroda thoughts. So, we should avoid that it is a real blocker if it is working in some environment like colima for local dev, but we should open issues to track where it is not working, and dedicate some time, when other big priorities are accomplished (like our next release) |
From what I understood from @akihikokuroda it seems that without those pins the images build so we can start trying to solve that. I'm going to need a couple more days for the next security steps so we have time to investigate it and fix it. @akihikokuroda if you unpin those libs, builds the image and inspect it, which version those libraries have installed? Thank you for the help! |
@Tansito It seems that these 2 packages are not necessary to build the |
@akihikokuroda can you try now just to confirm? 🤞 |
@Tansito They are built fine now. |
Thank you all for checking this, merging now 🙏 |
Summary
With the purpose to verify our security processes I started to implement checks in our infrastructure implementation. This PR implements the
hadolint
linter in all ourDockerfiles
.Details and comments
ref #690