-
Notifications
You must be signed in to change notification settings - Fork 11
CLOUDP-333181: Combined dockerfiles #289
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
base: master
Are you sure you want to change the base?
Conversation
Combined Dockerfiles for MongoDB Kubernetes Operator Combined Dockerfiles for Init Ops Manager image Combined Dockerfiles for Init Database and Init AppDB Combined Dockerfiles for Database Combined Dockerfiles for upgrade-hook and readinessprobe Added agent Dockerfiles Created Dockerfile for OpsManager Removed Dockerfile.dcar and README.md about how to build it Add more gitgraphs
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.
I think this file is not used anywhere (this why I have removed it), but please double check.
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.
same here, the LICENSE file was unused I think
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.
b: same here
inventories/database.yaml
Outdated
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.
b: we should not change anything in inventories .yaml files. Those changes were introduced by me to be able to generate final Dockerfiles. After this they are not needed
lib/sonar/sonar.py
Outdated
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.
file permission were changed so I can run the script manually, but this was a mistake. It should be reverted
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.
This script should be removed and only added in the PR description as information how we generated the Dockerfiles
@@ -6,13 +6,13 @@ images: | |||
- name: init-appdb | |||
vars: | |||
context: . | |||
template_context: docker/mongodb-kubernetes-init-database |
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.
@MaciejKaras Should we keep this renaming ?
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.
I would rather keep the same name here to prevent any changes. mongodb-kubernetes-init-appdb
can unused for now
@@ -0,0 +1,60 @@ | |||
ARG imagebase |
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.
For review: Dockerfile.old files are the copy of the initial Dockerfile
inventories/init_appdb.yaml
Outdated
@@ -22,7 +22,6 @@ images: | |||
- name: init-appdb-template-ubi | |||
task_type: dockerfile_template | |||
template_file_extension: ubi_minimal | |||
tags: ["ubi"] |
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.
lets keep this tag
I see that |
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.
Keep in mind that we should get rid of this image entirely CLOUDP-329374.
But we can merge it for now and make that change before 1.3
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.
LGTM
RUN CGO_ENABLED=0 GOFLAGS=-buildvcs=false go build -o /readinessprobe ./mongodb-community-operator/cmd/readiness/main.go | ||
RUN CGO_ENABLED=0 GOFLAGS=-buildvcs=false go build -o /version-upgrade-hook ./mongodb-community-operator/cmd/versionhook/main.go |
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.
nit: should we have these two go build
commands in just one RUN
statement? That would result into less layers and potentially smaller image.
Summary
Since we will stop re-building images daily, we won't have
-context
images anymore. We then don't need Dockerfiles templating.This PR creates self-sufficient Dockerfiles that build our images without templating. They only need variables passed with ARG.
For images which already had a plain Dockerfile, it was renamed
Dockerfile.old
, and inventories modified accordingly.For now, we keep templates and Sonar, until the Atomic Release epic is completed and we fully get rid of all of it.
Proof of Work
The proof of work will come when actually using them in a build pipeline. In a future PR.
This one is a noop, Dockerfiles are created but not used yet.
Methodology
The Dockerfiles were automatically generated, using Sonar templating, by adding special
final_dockerfile
tag to stages necessary to generate one. Using code like:Checklist