Skip to content
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

refactor: avoid Dockerfile duplication #1430

Merged

Conversation

ndr-brt
Copy link
Contributor

@ndr-brt ndr-brt commented Jul 18, 2024

WHAT

Currently we have multiple Dockerfile around the codebase that are pretty much the same, to avoid duplication I put one in the root folder and have the dockerize task to copy it in the project build folder.
That will make also the dependabot configuration slimmer

WHY

cleanup

FURTHER NOTES

  • deployment-test was calling dockerize for all the modules in the project, now it has been fixed to launch it only for required modules

Closes # <-- insert Issue number if one exists

@ndr-brt ndr-brt added the refactoring Refactoring, does not add functionality label Jul 18, 2024
@paullatzelsperger
Copy link
Contributor

paullatzelsperger commented Jul 18, 2024

I have my doubts about the merits of this PR overall because I don't see the advantage of replacing several docker file with a Gradle task. What exactly are we optimizing?

I would also dispute the notion that the Docker files are "duplicated", they simply are a build recipe for similar artefacts, not dissimilar to a build.gradle.kts.

More importantly though, this means, that docker images can only be built through Gradle then, which can be a problem. For example, this will break our Docker release process, because there we don't use the dockerize task, but we use several dedicated GH Actions to set metadata and push the image.
Is it really worth re-engineering the release process, just to reduce a few files?

@ndr-brt
Copy link
Contributor Author

ndr-brt commented Jul 18, 2024

I have my doubts about the merits of this PR overall because I don't see the advantage of replacing several docker file with a Gradle task. What exactly are we optimizing?

the gradle task is already there, I just centralized the Dockerfile (resulting a single file instead of 7 different ones).
Having a single file will facilitate version update but also make eventual changes easier: changing something in a single point is definitely easier than doing it 6 times.

I would also dispute the notion that the Docker files are "duplicated", they simply are a build recipe for similar artefacts, not dissimilar to a build.gradle.kts.

Our build files are providing different runtime flavours (with different dependencies), but the Dockerfile "logic" is the same over the different files: put the fat jar into it, handle configuration and run the application.

More importantly though, this means, that docker images can only be built through Gradle then, which can be a problem. For example, this will break our Docker release process, because there we don't use the dockerize task, but we use several dedicated GH Actions to set metadata and push the image. Is it really worth re-engineering the release process, just to reduce a few files?

There's no change in the build process: the dockerize task is still used, but instead of taking the file from the src/main/docker folder, it will rely on the copyDockerfile task, that will copy the file in the build folder, from which it will be taken to build/publish the image.

@ndr-brt ndr-brt force-pushed the remove-duplicated-dockerfile branch 3 times, most recently from db186b6 to 5dcbc86 Compare July 18, 2024 12:38
@ndr-brt ndr-brt marked this pull request as ready for review July 18, 2024 12:47
@ndr-brt ndr-brt force-pushed the remove-duplicated-dockerfile branch from 5dcbc86 to 29d29f2 Compare July 18, 2024 12:47
@paullatzelsperger
Copy link
Contributor

paullatzelsperger commented Jul 18, 2024

There's no change in the build process: the dockerize task is still used, but instead of taking the file from the src/main/docker folder, it will rely on the copyDockerfile task, that will copy the file in the build folder, from which it will be taken to build/publish the image.

The docker release process (to dockerhub) does not use dockerize, that is my point. Checkout publish-new-release.yaml and from there actions/publish-docker-image. Instead, we're using dedicated GH Actions for this, that build, set metadata and publish.

we'd have to also execute ./gradlew copyDockerfile before we run the docker/build-push-action@v3 action and adapt the paths therein.

@ndr-brt
Copy link
Contributor Author

ndr-brt commented Jul 19, 2024

There's no change in the build process: the dockerize task is still used, but instead of taking the file from the src/main/docker folder, it will rely on the copyDockerfile task, that will copy the file in the build folder, from which it will be taken to build/publish the image.

The docker release process (to dockerhub) does not use dockerize, that is my point. Checkout publish-new-release.yaml and from there actions/publish-docker-image. Instead, we're using dedicated GH Actions for this, that build, set metadata and publish.

we'd have to also execute ./gradlew copyDockerfile before we run the docker/build-push-action@v3 action and adapt the paths therein.

oh, yes, I got it wrong, anyway the copyDockerfile has been put as a dependency of the shadowJar task, so it gets executed before every publish anyway.

I mean, in the publish-docker-image we're using gradle anyway (to build the jar), why don't leverage it and have a unique process to do that?

I also noticed, in publish-docker-image:

I guess that happened because of the duplication, which is always bad and we should avoid it as much as possible

Copy link
Contributor

@paullatzelsperger paullatzelsperger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor nit: if the docker file should remain at the repo root I would call it Dockerfile-template or something, just to make it clear and avoid confusion

@ndr-brt ndr-brt force-pushed the remove-duplicated-dockerfile branch 3 times, most recently from 30a0e95 to 3a4f0c2 Compare July 23, 2024 10:03
@ndr-brt ndr-brt force-pushed the remove-duplicated-dockerfile branch 2 times, most recently from 7b3e6b0 to a3c3137 Compare July 23, 2024 10:22
@ndr-brt ndr-brt force-pushed the remove-duplicated-dockerfile branch from a3c3137 to 5ec6dde Compare July 23, 2024 10:34
@ndr-brt ndr-brt force-pushed the remove-duplicated-dockerfile branch from 0212561 to d2a70ec Compare July 23, 2024 12:45
Copy link

sonarcloud bot commented Jul 23, 2024

@ndr-brt ndr-brt merged commit e5d11d1 into eclipse-tractusx:main Jul 23, 2024
33 checks passed
@ndr-brt ndr-brt deleted the remove-duplicated-dockerfile branch July 23, 2024 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Refactoring, does not add functionality
Projects
Status: Merged
Development

Successfully merging this pull request may close these issues.

2 participants