Update Dockerfile to use multi-stage builds#4746

Closed

heliocastro wants to merge 43 commits into

oss-review-toolkit:mainfrom

heliocastro:docker_overhaul

heliocastro commented Nov 24, 2021 •
edited

Loading

Contributor

Uses multi-stage build to reduce final container
Harmonize the base image to use same OpenJDK base from Eclipse Temurin and Ubuntu Focal LTS
Make all tooling versions customizable through docker build args
Each tooling is built in separate components, improving the process to add or remove specific tools

heliocastro requested a review from a team as a code owner

November 24, 2021 08:06

tsteenbe requested changes

View reviewed changes

docker/python.sh Outdated

docker/ort-wrapper.sh Outdated

docker/python.sh Outdated

scripts/docker_build.sh Outdated

Dockerfile Outdated

docker/android.sh Outdated

docker/nodejs.sh Outdated

heliocastro force-pushed the docker_overhaul branch 3 times, most recently from efbd9e1 to dace4b4 Compare

November 24, 2021 11:23

maxhbr reviewed

View reviewed changes

maxhbr left a comment

Contributor

just some comments

Dockerfile Outdated

docker/python.sh Outdated

docker/bash_bootstrap.sh

Dockerfile Outdated

Dockerfile Outdated

heliocastro force-pushed the docker_overhaul branch 4 times, most recently from c93e88f to 998daa5 Compare

November 24, 2021 13:17

heliocastro requested review from tsteenbe and removed request for a team

November 24, 2021 13:19

tsteenbe requested changes

View reviewed changes

Dockerfile Outdated

Dockerfile Outdated

Dockerfile Outdated

Dockerfile Outdated

Dockerfile Outdated

Dockerfile Outdated

Dockerfile Outdated

Dockerfile Outdated

+                  && rbenv global ${RUBY_VERSION} \
+                  && gem install bundler cocoapods
+              # Install 'CocoaPods'. As https://github.com/CocoaPods/CocoaPods/pull/10609 is needed but not yet released.

tsteenbe Nov 24, 2021

Member

CocoaPods/CocoaPods#10609 was merged and I believe also released?

heliocastro Nov 25, 2021

Contributor Author

Maybe, but i do not want change this since is this way on upstream Dockerfile. If we merge, then in later step i change to newer version of CocoaPods.

sschuberth Feb 22, 2022

Member

Please see again how this is now done in the "legacy" Dockerfile.

Dockerfile Outdated

Dockerfile Outdated

heliocastro force-pushed the docker_overhaul branch 3 times, most recently from 0106250 to 25d0408 Compare

November 25, 2021 07:37

heliocastro requested a review from tsteenbe

November 25, 2021 07:50

This comment has been minimized.

Sign in to view

maxhbr reviewed

View reviewed changes

Dockerfile Outdated

heliocastro force-pushed the docker_overhaul branch from ef532fa to dfe9dcc Compare

November 26, 2021 09:34

maxhbr reviewed

View reviewed changes

Dockerfile Outdated

Dockerfile Outdated

Dockerfile Outdated

Dockerfile Outdated

Dockerfile Outdated

maxhbr reviewed

View reviewed changes

docker/bash_bootstrap.sh Outdated

heliocastro force-pushed the docker_overhaul branch 3 times, most recently from 3ce67da to 1e59dca Compare

December 3, 2021 10:56

heliocastro force-pushed the docker_overhaul branch 5 times, most recently from c219a54 to 319991d Compare

December 9, 2021 09:06

Helio Chissini de Castro added 22 commits

September 8, 2022 14:59


          Docker update: Revert to Java 11

1debeb1

Signed-off-by: Helio Chissini de Castro <helio.chissini-de-castro@bmw.de>


          Docker update: Update cocoapods version

4cb4b94

Signed-off-by: Helio Chissini de Castro <helio.chissini-de-castro@bmw.de>


          Docker update: New SBT ver. Improve nvm handling

797bd6d

Signed-off-by: Helio Chissini de Castro <helio.chissini-de-castro@bmw.de>


          Docker update: Improve user creation

acd1c7a

- User, user id and group id can now be passed as ARGs, USERNAME,
  USER_ID and USER_GID rescpectively
- Moved the order of main container package install to allow sudo been
  enabled

Signed-off-by: Helio Chissini de Castro <helio.chissini-de-castro@bmw.de>


          Docker update: Sync with upstream build script

c8fe7ce

Signed-off-by: Helio Chissini de Castro <helio.chissini-de-castro@bmw.de>


          Docker fix: Don't ignore dockerignore

91d3fca

Signed-off-by: Helio Chissini de Castro <helio.chissini-de-castro@bmw.de>


          Docker update: Install CocoaPods direct from GEM now

b3e1477

Signed-off-by: Helio Chissini de Castro <helio.chissini-de-castro@bmw.de>


          Docker update: Add Python2 option and Scancode as pip

466811e

- Python default version is 3.10.2
- Python2 can be optionally installed through build-arg PYTHON2_VERSION
- Scancode build was removed in favor of pip based version

Signed-off-by: Helio Chissini de Castro <helio.chissini-de-castro@bmw.de>


          Docker fix: run locale-gen one time only

bdd38d7

Signed-off-by: Helio Chissini de Castro <helio.chissini-de-castro@bmw.de>


          Dockerfile: Update GRADLE_HOME and Bower

73a0521

Signed-off-by: Helio Chissini de Castro <helio.chissini-de-castro@bmw.de>


          Dockerfile: Add missing openssl on Ubuntu install

604c239

Signed-off-by: Helio Chissini de Castro <helio.chissini-de-castro@bmw.de>


          Dockerfile: Update legacy dockerfile

cc45df2

Signed-off-by: Helio Chissini de Castro <helio.chissini-de-castro@bmw.de>


          Dockerfile: Add tooling versions in docker_build.sh to easy updates

826acfd

Signed-off-by: Helio Chissini de Castro <helio.chissini-de-castro@bmw.de>


          docker: Update to Ubuntu latest LTS ( Jammy )

14d8d13

- Update Ruby to 3.1.2 ( ssl3 )
- Update PHP composer to latest using installer
- Update PIP
- Move work user to Ort to be Batect compliant
- Update NodeJS for current LTS

Signed-off-by: Helio Chissini de Castro <helio.chissini-de-castro@bmw.de>


          Docker: Allow user to decide which buildArgs is needed passing on com…

cb88aac

…mand line

Signed-off-by: Helio Chissini de Castro <helio.chissini-de-castro@bmw.de>


          batect: Remove enable-buildkit as new docker has it embedded

27f3c30

Signed-off-by: Helio Chissini de Castro <helio.chissini-de-castro@bmw.de>


          docker: Add python-inspector and update python patch version

b39363a

Signed-off-by: Helio Chissini de Castro <helio.chissini-de-castro@bmw.de>


          docker: Small fixes

259f6e0

- Move python packages to last stage to redice big copy and permission
  long chmod
- Change android sdk permissions
- Install pnpm ( missing )
- Update npm version
- Fix pyenv version

Signed-off-by: Helio Chissini de Castro <helio.chissini-de-castro@bmw.de>


          docker: Make batect understand entrypoint

9af1ac0

- Add locale on image defaults to en_US.UTF-8

Signed-off-by: Helio Chissini de Castro <helio.chissini-de-castro@bmw.de>


          docker: Add buildkit back to batect

980c7cd

Signed-off-by: Helio Chissini de Castro <helio.chissini-de-castro@bmw.de>


          docker: Restore python to build stage and fix versions for pipenv and…

3afb2ba

… poetry

Signed-off-by: Helio Chissini de Castro <helio.chissini-de-castro@bmw.de>


          docker: Minor fixes

fd06d32

- Fix android sdk folder permissions
- Add buildx as default docker engine build and add implicit amd64 to
  build on non x86 based systems

Signed-off-by: Helio Chissini de Castro <helio.chissini-de-castro@bmw.de>

tsteenbe approved these changes

View reviewed changes

sschuberth commented Sep 8, 2022

Member

Cross-posting from Slack: Please don't merge this with a whopping 43 (!) commits in the history. I guess most of this should be squashable, or?

nicorikken mentioned this pull request

JAVA_HOME is set to an invalid directory alliander-opensource/ort-container#11

Open

sschuberth requested changes

View reviewed changes

sschuberth left a comment

Member

Sorry, the Git history in this PR is a mess. I'm blocking the merge until the TSC has decided on how to proceed.

Dockerfile

               ENV LC_ALL=en_US.UTF-8
-              RUN echo $LANG > /etc/locale.gen
+              RUN echo $LANG > /etc/locale.gen \

sschuberth Sep 8, 2022

Member

How is this related to "Make batect understand entrypoint"?

sschuberth commented Sep 8, 2022

Member

@mnonnenmacher @MarcelBochtler @fviernau could you live with the Git history in this PR?

mnonnenmacher commented Sep 8, 2022 •
edited

Loading

Member

@mnonnenmacher @MarcelBochtler @fviernau could you live with the Git history in this PR?

@sschuberth I would not merge it like this, because just from a short look I see so many issues:

43 commits is far too much for a single PR. This is impossible to review.
Many commits change multiple things at once. Just as one example, the "Revert to Java 11" commit changes the pyenv installation and there is no explanation in the commit message if this is related to the base image change or just completely unrelated.
There are a lot of typos and hard to understand sentences in the commit messages.
Several commits contain lists of changes which shows they should have been separate commits.
There is one commit that creates a legacy Dockerfile, but this is not the current version of the file but an old version. Instead, there is a separate commit that updates the legacy file.
There are version updates all over the place, it's unfeasible to check if all of these are updates related to some old version of the original Dockerfile or to the current Dockerfile. For example, the first commit downgrades ScanCode only to upgrade it again in a later commit.
Commit message prefixes are inconsistent, some start with "Docker fix:" or "Docker update:" or "Dockerfile:" or "docker:".
It's unclear why some entries were added to the .gitignore.
The link to the BuildKit docs was removed from the README.

And this is not the complete list.

I understand that this PR was worked on for almost a year now, and that this was probably a bit frustrating. So I want to thank @heliocastro for being to persistent with your efforts to improve our Docker setup. But nevertheless this PR should follow our standards. We have put a lot of effort into maintaining a clean Git history for this project because it brings a lot of benefits, and I would not like to ignore this now just because someone has an urge to merge these changes.

My suggestion to move this forward in a clean way would be to squash all changes into just a few commits, so that the PR can be reviewed properly, for example:

One commit to copy the current Dockerfile to the legacy directory.
A few commits that apply the relevant changes on top of the current Dockerfile, but without all the version updates that just repeat version updates which already happened in our current Dockerfile. Or even squashing all changes into a single commit would be better than the current state of the PR.
One commit for each version update that changes a version compared to our current Dockerfile.

@sschuberth @fviernau @MarcelBochtler Does one of you maybe have a better idea how to clean this up?

And as a general comment, if you work on a contribution that takes a long time, to a project with mandatory code review, it is super important to rebase your changes on top of the current main branch on a regular basis. Yes, this can be a lot of work, but it is the responsibility of the author to make sure that their changes are in a reviewable state.

sschuberth commented Sep 9, 2022

Member

Does one of you maybe have a better idea how to clean this up?

Looks like a clean-up was already done in #5760. I'll close this PR in favor of that one. Let's continue the discussion over there.

sschuberth mentioned this pull request

Docker: Component based Multi stage docker #5760

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet