Fix all dockerlint warnings/info's?

[Macleykun]

Hello there!

I've been lurking here since the first video of dave, i lack really any proper experience in any (programming) language, but always loved working with Docker!

While lurking, i noticed the dockerlint you guys use, and today i finnaly want to propose that i can look into these warnings/info's if it's useful/desired!

I created a pretty bad simple shell script:

#!/bin/sh
find . -type f \( -iname "Dockerfile" \)|while read FNAME; do
  LINT=$(cat "$FNAME" | docker run --rm -i hadolint/hadolint)
  if [[ "$LINT" == *"DL"* ]];
    then
    echo "$FNAME"
    echo "$LINT"
    echo;
  fi
done

I admit, using the docker way for hadolint makes it quite slow, but i'll paste the results here, to spare you the time.

There's:
63 incompliant Dockerfiles
57 warning
112 info level

If you want to know per code what the amounts are, you can see that here:

DL3008	DL3009	DL3015	DL3018	DL3019	DL3047	DL3059	DL4006	EndTotal
30	21	30	24	9	8	44	3	169

It was already fun to find out what lint tool ya guys are using, make a script and optimize the results with shell & excel :P

There might be some DL's that can be fixed by some simple replacing. DL3059 has issues like:

RUN apt-get update
RUN apt-get install -y build-essential

I probably would check them manually as 63 files seems like a lot, but doable in a few hours.

Just let me know if you guys want me to look into this, probably will work on this daily, more so in the weekends until it's all done.

Also say hi from me to dave :P haha

[rbergen]

@Macleykun That would be useful, indeed. As with most anything, the best way to do so in my opinion is to open a PR with the changes you propose to make. You seem to want to do this in a phased manner. I understand that and it's fine in principle, but the repo will be in motion while you update the Dockerfiles. That means that you may have to solve merge conflicts due to other people's PRs being merged, and/or you may want to cut up your changes into more than one PR to prevent that.

Also say hi from me to dave :P haha

We don't have to if you contribute. He really does keep an eye on this project he started. :)

[Macleykun]

I believe it’s best to take 5 dockerfiles at a time and scale up to 10 if it’ll go well. You want me to also use only one type of image? Like the debian:stable-slim that @mike-barber proposed one time. Once the standard base image is established, I’ll take inspiration of the given dockerfiles and try to minimize steps and size (copying only files that are needed). You want for each a before and after score? (Have to do tests anyways for each). Met vriendelijke groet, Wesley de Vree

…

________________________________ Van: Rutger van Bergen ***@***.***> Verzonden: Tuesday, September 7, 2021 9:36:05 AM Aan: PlummersSoftwareLLC/Primes ***@***.***> CC: Macley ***@***.***>; Mention ***@***.***> Onderwerp: Re: [PlummersSoftwareLLC/Primes] Fix all dockerlint warnings/info's? (#708) @Macleykun<https://github.com/Macleykun> That would be useful, indeed. As with most anything, the best way to do so in my opinion is to open a PR with the changes you propose to make. You seem to want to do this in a phased manner. I understand that and it's fine in principle, but the repo will be in motion while you update the Dockerfiles. That means that you may have to solve merge conflicts due to other people's PRs being merged, and/or you may want to cut up your changes into more than one PR to prevent that. Also say hi from me to dave :P haha We don't have to if you contribute. He really does keep an eye on this project he started. :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#708 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AGJIY42K62STPAXOHFXTSU3UAW6GLANCNFSM5DRYMH4Q>. Triage notifications on the go with GitHub Mobile for iOS<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[mike-barber]

Initial thoughts are that a standard base image could work well for solutions that are building from scratch, but I'd avoid it for the ones that are using a standard image appropriate for the language, e.g. Rust or C# are the two that I'm familiar with.

Solution authors can potentially be picky about the exact distro they're using sometimes: they might have specific versions of compilers they've been doing development with.

It'll be really good cleaning up some of the multiple RUN stuff. I think I'm guilty too :)

[Macleykun]

Actually, if we’re speaking about specific compiler versions. I already have to do that as some lint errors are about that. I’ll check which compiler version Is used and if I can get that on Debian aswell, then technically only the OS changed.

Also, perhaps it's better to use the alpine images? They are known to be the smallest :P

To answer my own question to include before/after scores. Here is the score before my changes: Passes: 13540, Time: 5.000125871, Avg: 0.000369285, Limit : 1000000, Count1 : 78498, Count2: 78498, Valid :TRUE BoopBeepBoopBeep; 13540; 5.000125871;1;algorithm=base,faithful=no After ``` Passes: 13407, Time: 5.000023362, Avg: 0.000372941, Limit : 1000000, Count1 : 78498, Count2: 78498, Valid :TRUE BoopBeepBoopBeep; 13407; 5.000023362;1;algorithm=base,faithful=no ``` Second time after ``` Passes: 13590, Time: 5.000006402, Avg: 0.000367918, Limit : 1000000, Count1 : 78498, Count2: 78498, Valid :TRUE BoopBeepBoopBeep; 13590; 5.000006402;1;algorithm=base,faithful=no ``` So i won't include any before/after tests. I also will try to make each dockerfile i pass into a build and then run phase. I don't think specifying AS RUN is needed unless you prefer otherwise. Not sure if this improves pulling images/calculating primes, but we'll see.

Do you wish that each dockerfile has a build phase, followed by a RUN phase?

Current pull: #710

…

I'll assume that all new pull's will be asked to change the dockerfile accordingly to fix all lints and possibly have a build phase and run phase. If you want.

[yellowcub]

Prior to this I had never used Docker, and had to do a lot of learning on the fly (@rbergen can attest I had to do a lot of GitHub learning here as well 😕😅).
If there are any Docker issues with the solution I have worked (PrimeSwift), I would like an opportunity to fix (with some guidance) for my own learning purpose.

[mike-barber]

Watch out for Alpine. Some of us have run into performance issues with it. It doesn't have a normal libc and it's a lot slower in some cases, but perfectly fine for others.

[rbergen]

@Macleykun The point of Dockerfile linting, aside from identifying "obvious errors", is mainly to a) optimize resource usage, primarily by reducing image size and layer count, and b) increase determinism by pinning down dependency versions etc. I think the "bulk changes" to Dockerfiles should be focused on that, as well.

In that context, my (strong) advice would be:

• don't change the fundamental composition of Dockerfiles. That includes their base image, build tooling, use of BUILD vs RUN images (or not), etc. As @mike-barber pointed out, contributors care about these things, and with reason. Phrased differently, in cases where the fundamental composition is changed, we would have to seek sign-off from the respective solution's contributor before merging, which will significantly slow down the application of your work
• when pinning down versions, pin them to the versions that the "undecorated" package names resolve to "now".

Also, please don't open draft PRs. By design, reviewing them is meaningless, and they muddle the PR list for maintainers. (I know this may sound a bit awkward with just over a handful of open PRs, but bringing the PR list to that point and keeping it there takes real work. Also, it can change within a day, for instance when @davepl releases another video on the project.)

[Macleykun]

Sure only focus on lint :)
Sorry about the draft, though it was ok (doesn’t need a review bc it’s draft I though). I’ll close it in a bit and reopen when she’s ready!
With versions I will do exactly that :)

[Macleykun]

Prior to this I had never used Docker, and had to do a lot of learning on the fly (@rbergen can attest I had to do a lot of GitHub learning here as well 😕😅).
If there are any Docker issues with the solution I have worked (PrimeSwift), I would like an opportunity to fix (with some guidance) for my own learning purpose.

oh don't worry, it'll be fun to see what issues i run into, and figure it out on my own :P

[Macleykun]

Good news, i did all the programming languages already: https://github.com/Macleykun/Primes/tree/drag-race2
I will have to wait until the first pull is merged, from there i'll resolve some conflicts from that pull and hopefully we can merge the second soonish :P (i know it's alot of changes i'll be pushing, and some probably are a bit changed in build (for example, multiple COPY's only to exclude two or so files that save a few KB's aren't worth it in my eyes, and just using COPY . . is better. I normally would advice to always do a two step but as promised, don't wanna change the build way to much).

The dockerlint part 2 will only focus what's done and not focus on any newer languages that have come. Part 3 will be the final one that will be alot smaller interms of changes but will make everything on the repo up-to-date and compliant.

From there it's up to the maintainers/contributers if they want to keep being 100% dockerlint compliant. I don't know if it's smart for me to always keep an eye on each pull and fix the dockerlint myself.