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

Initial public version of the Mongo-Initializr #1

Merged
merged 14 commits into from
Sep 26, 2024

Conversation

JacobusXIII
Copy link
Contributor

Added the Mongo-Initializr scripts and some helper scripts for building the Docker images.

Added the `Mongo-Initializr` scripts and some helper scripts for building the Docker images.
Gijsvanhorn
Gijsvanhorn previously approved these changes Aug 7, 2024
Copy link

@Gijsvanhorn Gijsvanhorn left a comment

Choose a reason for hiding this comment

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

LGTM

* Remove the ARG instructions from the Dockerfile. We don't need them here.
* Distinguish between the Docker entrypoint and the Mongo-Initializr script. It makes more sense to execute the Mongo-Initializr during the Docker build instead of a Docker entrypoint.
* Update the README.md
SerhatG
SerhatG previously requested changes Aug 8, 2024
Copy link
Member

@SerhatG SerhatG left a comment

Choose a reason for hiding this comment

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

I tried some stuff.. mostly some formatting/linting or stuff that worked well for me in bash scripting (which makes it also more readable for non-shells scripting people).
Though I didn't do it everywhere, just when I was making a change, I updated the formatting as well.

I didn't check some Docker construct fully yet.

bin/include.functions.sh Outdated Show resolved Hide resolved
bin/include.functions.sh Outdated Show resolved Hide resolved
bin/mi-build.sh Outdated Show resolved Hide resolved
bin/mi-build.sh Outdated Show resolved Hide resolved
bin/mi-build.sh Outdated Show resolved Hide resolved
docker/mongo-initializr.sh Outdated Show resolved Hide resolved
docker/mongo-initializr.sh Outdated Show resolved Hide resolved
docker/mongo-initializr.sh Outdated Show resolved Hide resolved
docker/mongo-initializr.sh Outdated Show resolved Hide resolved
docker/mongo-initializr.sh Outdated Show resolved Hide resolved
bin/mi-dbdata-sync.sh Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
COPY docker/docker-entrypoint.sh /
COPY bin/ "${MI_BIN_FOLDER}"

ENTRYPOINT ["/docker-entrypoint.sh"]
Copy link
Member

Choose a reason for hiding this comment

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

This overrides the default mongo entrypoint: https://github.com/docker-library/mongo/blob/e3137ef2ab515a4b7b02487ec72cf3a02d14b667/7.0/docker-entrypoint.sh

Which contains some fixes and workarounds and also does stuff like creating default database and user/passwords based on ENV variables. For the sake of compatibility we would need to call that one somewhere unless it breaks some of the things we do.

Our entrypoint seems to point directly to mongo-initializr.sh. Technically we could remove ours and just use the as a CMD, or use a RUN construct as done in the example.

Copy link
Contributor Author

@JacobusXIII JacobusXIII Aug 12, 2024

Choose a reason for hiding this comment

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

I don't like depending on the original entrypoint script. I don't fully understand the script and I'm not sure if it can cause any problems.

We should run our script as ENTRYPOINT.
In the original base image mongod is set as CMD what I would like to keep as is.
And the disadvantage of a RUN is that it cannot be overwritten like the ENTRYPOINT.

bin/mi-dbdata-sync.sh Outdated Show resolved Hide resolved
bin/mi-sync.sh Outdated Show resolved Hide resolved
@SerhatG
Copy link
Member

SerhatG commented Aug 9, 2024

If this container is build using the example.. How does it start the database?
If I read it correctly.. it will run mongo-initializr.sh which will only start mongo if USERNAME/PASS are set and you want to do a build.

@JacobusXIII
Copy link
Contributor Author

If this container is build using the example.. How does it start the database? If I read it correctly.. it will run mongo-initializr.sh which will only start mongo if USERNAME/PASS are set and you want to do a build.

The database is started using the CMD instruction in the mongo base image.

@SerhatG
Copy link
Member

SerhatG commented Aug 13, 2024

If this container is build using the example.. How does it start the database? If I read it correctly.. it will run mongo-initializr.sh which will only start mongo if USERNAME/PASS are set and you want to do a build.

The database is started using the CMD instruction in the mongo base image.

If I understand correctly the CMD is passed to the entrypoint as extra arguments, which the entrypoint can choose to execute or wrap around. That's why most entrypoints end with a call like this.

Overview with how ENTRYPOINT and CMD interact:
https://docs.docker.com/reference/dockerfile/#understand-how-cmd-and-entrypoint-interact

Didn't know there was a difference between ENTRYPOINT a b and ENTRYPOINT ["a", "b"]. 😱

Though I also just read a note on that page saying it gets reset in our particular case..

If CMD is defined from the base image, setting ENTRYPOINT will reset CMD to an empty value. In this scenario, CMD must be defined in the current image to have a value.

* Use Mongo entrypoint to initialize MongoDB instead of doing it ourself.
* The MI-bin, -source and -dbdata folder are not customizable anymore and located in the `/mi` root folder which is created during the Docker build stage.
* We havent figured out how we still can build the database during the Docker build yet. We have therefore removed this from the `README.md`.
* Fixed soms cut/paste errors. `MI_DATABASE_USERNAME` and `MI_DATABASE_PASSWORD` where not set correct.
Make it possible to initialize the database during th docker build.
During the Docker build, the Mongo database can be initialized and dumped. On startup the dumped database will be restored. The `Mongo-Initializr` script takes care of this all.
@JacobusXIII
Copy link
Contributor Author

@SerhatG It is now possible to initialize the database during the docker build. Can you please review the code again?

@JacobusXIII
Copy link
Contributor Author

@SerhatG It is now possible to initialize the database during the docker build. Can you please review the code again?

See Dockerfile for initializing the database during the Docker build. in README.md.

README.md Outdated Show resolved Hide resolved
@Gijsvanhorn Gijsvanhorn dismissed their stale review September 24, 2024 07:55

Much has changed since first review

Copy link

@wtenbosch wtenbosch left a comment

Choose a reason for hiding this comment

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

LGTM
I don't see any apparent issues (but I also don't have a lot of shell scripting experience)

@JacobusXIII JacobusXIII merged commit d405390 into aerius:main Sep 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants