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

DBZ-1279 Dockerfile improvements #131

Merged
merged 9 commits into from May 30, 2019

Conversation

Projects
None yet
3 participants
@renatomefi
Copy link
Member

commented May 28, 2019

Context #127

I have one issue now is that the base script for downloading maven dependencies lives in the connect-base image, thus it has to be tagged before we can build the connect one. How can we fix that?

@gunnarmorling

This comment has been minimized.

Copy link
Member

commented May 28, 2019

thus it has to be tagged before we can build the connect one. How can we fix that?

Does it have to be fixed actually? It seems as if it's just the "natural" flow of things, that the base image must be built before? In fact, it was the same before, I'd say?

@gunnarmorling

This comment has been minimized.

Copy link
Member

commented May 28, 2019

Ah, I begin to understand, this is about the issue on CI, right? @jpechane, I think this reveals a general issue: the CI build doesn't use the base images built by itself before, but it does a remote fetch (as the images are tagged with "latest", whereas the dependency version is a fixed thing like 0.9).

@renatomefi

This comment has been minimized.

Copy link
Member Author

commented May 28, 2019

@gunnarmorling It does not indeed, but the CI will fail in this case, but if you can test locally:

$ docker build -t debezium/connect-base:0.9 connect-base/0.9
$ docker build -t debezium/connect:0.9 connect/0.9
$ docker run --rm -it debezium/connect:0.9 sh -c "ls /opt/kafka/connect"
# Check the debezium plugins
$  docker run --rm -it debezium/connect:0.9 sh -c "ls /kafka/libs"
# Check the dependency jars, like confluent and avro, jackson, etc.
@renatomefi

This comment has been minimized.

Copy link
Member Author

commented May 28, 2019

Ah, I begin to understand, this is about the issue on CI, right? @jpechane, I think this reveals a general issue: the CI build doesn't use the base images built by itself before, but it does a remote fetch (as the images are tagged with "latest", whereas the dependency version is a fixed thing like 0.9).

Also yes, it's easy to fix tho, should I?

@renatomefi renatomefi force-pushed the renatomefi:docker/dockerfile-improvements-2 branch from 9bde416 to 5e64d03 May 28, 2019

Show resolved Hide resolved connect/0.10/Dockerfile Outdated
@gunnarmorling
Copy link
Member

left a comment

LGTM! Thanks a lot, @renatomefi!

@jpechane, any objections? Are you fine with adding it for 0.10.0.Alpha1?

@jpechane

This comment has been minimized.

Copy link
Contributor

commented May 28, 2019

@gunnarmorling Let's do it after alpha1 - just in case release pipeline will need updating.

@gunnarmorling

This comment has been minimized.

Copy link
Member

commented May 29, 2019

Good to merge it now, @jpechane?

@jpechane

This comment has been minimized.

Copy link
Contributor

commented May 29, 2019

@renatomefi LGTM! Could you please just rebase to the latest master?

renatomefi added some commits May 14, 2019

DBZ-1279 Introduce Dockerfile linter with Hadolint
Also add it as `before_script` in the Travis build
DBZ-1279 Fix DL4000 - MAINTAINER is deprecated
https://github.com/hadolint/hadolint/wiki/DL4000

It's now replaced with LABEL where the value is maintainer
MAINTAINER is deprecated since Docker 1.13.0 hadolint/hadolint#71.
DBZ-1279 Rewrite `connect` Dockerfile
Only included versions 0.9 and 0.10 since they are the current ones
being built and the bases for the future ones.

The reasons behind it starts with:
./connect/0.10/Dockerfile:13 SC2039 In POSIX sh, brace expansion is undefined.
./connect/0.10/Dockerfile:13 SC2039 In POSIX sh, indirect expansion is undefined.
./connect/0.10/Dockerfile:13 SC2086 Double quote to prevent globbing and word splitting.
./connect/0.10/Dockerfile:13 DL4006 Set the SHELL option -o pipefail before RUN with a pipe in it

Now we have a simpler and future proof approach, where we do not define
complex shell structures within the RUN command, having a simple shell
scripts is a valida solution which is also easier to test.
DBZ-1279 Validate images upon build
This way we only validate those which are part of the current build,
also keeping compatibility with the Jenkins auto update by not adding
extra scripts with Debezium versions.
Another reason is to avoid to check the old versions.
The hadolint version is kept as `latest` on purpose, since it's a linter
we want to have access to its latest features and be able to update the
Dockerfiles timely for the upcoming backwards compatibility breaks
DBZ-1279 Improve build-all.sh script
With `set -eo pipefail` it isn't necessary to manually check for errors,
the script will fail correctly when any of the scripts do as well
DBZ-1279 Add maven dep download script
This script will cover the needs to download dependencies from maven,
they cancome from central, confluent or debezium.
Also centralize Kafka ENV variable sin connect-base for easier
maintenance.
DBZ-1279 Ensure new builds are chained via tags
Before this change the build process would download the tag from
Docker hub, which is not necessarily the image being build at the
moment, this is now fixed by tagging it corretly locally and not
depending on the registry for that.

@renatomefi renatomefi force-pushed the renatomefi:docker/dockerfile-improvements-2 branch from 5e64d03 to 41b285c May 29, 2019

@renatomefi

This comment has been minimized.

Copy link
Member Author

commented May 29, 2019

@jcechace done!

@jpechane jpechane merged commit 036c6c1 into debezium:master May 30, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jpechane

This comment has been minimized.

Copy link
Contributor

commented May 30, 2019

@renatomefi Excellent work! Thanks a lot

@renatomefi renatomefi deleted the renatomefi:docker/dockerfile-improvements-2 branch Jun 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.