jenkinsci / docker

Docker official jenkins repo
https://hub.docker.com/r/jenkins/jenkins
MIT License
6.73k stars 4.55k forks source link

impr(optimization/refactor): Optimized Debian dockerfiles, Enhanced readability + Enhanced code structure. (@Ilolm) #1948

Open ilolm opened 3 weeks ago

ilolm commented 3 weeks ago

Description

1.Reduced layeres amount. 2.Enhanced readibility. 3.Enhanced dockerfile sctructure(Moved variable in one place instead of being scattered on the rest of the code). 3.Fixed some bugs such as lowercase 'as'.

Testing done

1. Manually built an edited Dockerfile image successfully.

docker image built --no-cache -t test_feature .

2. Ran the image successfully and checked the web ui.

docker container run --rm -p 8080:8080 -p 50000:50000 test_feature

Submitter checklist

dduportal commented 3 weeks ago

Additionally:

ilolm commented 3 weeks ago

Hi there! Firstly I highly appreciate your review and notices made about the changes.


This PR does a LOT of things. It's really hard, as a maintainer committed to have a safe and sustainable image, to review and evaluate all risks. It's not immediate which change solves which problem. Do you mind splitting in different PRs, each one solving a single problem (so easier to review, faster to merge when no discussion is needed)? Or at least open distinct issues explaining the problem and the solving strategy so we could relate and discuss (instead of having what is going to be a really big PR slow to review and merge with tons of discussions)?

Okay, I will take this into account.


Grouping ARG and ENV does not change the layering at all, because it's only a set of metadata (docker build does not create any layer with these instructions). Besides, it introduces big downsides in this repository:

The main purpose of this was to made the code structure more readable by moving each piece of code into his own section. However if it breaks the code or that was made specifically for caching purposes, I will modify those things back in my repo.


Moving the LABEL on top is breaking the values (as it uses ARG values which are defined later in the scopes).

Yeah, that's definitely my bad, sorry about that. It's gonna be fixed as well.


Mixed feelings about the COPY instruction in its array form. It does not reduce layers in the context of docker buildx, but it simplifies the copy. It feels a source of confusion as it requires higher skills in Linux + Docker skills. I'm torn between the simplification vs. making it harder to read and contribute

As for me it looks really self-explanatory but if you want, I can change it back as well :)


I also agree with your personnel notices --> so, there are gonna be fixed as well.


Is there a reason to have done these changes only on the debian images?

No, I was going to make the same task with the alpine images later.


The linter (hadolint) fails (as per the CI checks) which is a prerequisite

Yeah, I see. I'm gonna fix it as well

ilolm commented 3 weeks ago

Hadolint fix is in progress

ilolm commented 3 weeks ago

hanolint couldn't parse combined ENV/ARG statements, that's why it was failing

ilolm commented 3 weeks ago

Summarizing changes:

dduportal commented 2 weeks ago

Thanks @ilolm ! I'm a bit busy these time, so expect some delay in the review