feat(DBZ-7551): avoid overriding MAVEN_DEP_DESTINATION in connect image #367
Conversation
|
@razvanz Hi, this change breaks the release process, please see https://github.com/debezium/debezium/blob/main/jenkins-jobs/pipelines/release/release-pipeline.groovy#L489 |
From what I gather, only these 2 lines are breaking. |
I don't see any immediate issue, but I defer to @jpechane here. One thing I will add, though, is this should be recorded with a Jira issue and marked with an "add to migration guide" label. This guarantees that we at least add something to the release notes about this change in the smallest chance that users may have their own descendent builds that rely on the directory of |
| @@ -17,6 +17,7 @@ set -e | |||
| MAVEN_REPO_CENTRAL=${MAVEN_REPO_CENTRAL:-"https://repo1.maven.org/maven2"} | |||
| MAVEN_REPOS_ADDITIONAL=${MAVEN_REPOS_ADDITIONAL:-""} | |||
| MAVEN_REPO_CONFLUENT=${MAVEN_REPO_CONFLUENT:-"https://packages.confluent.io/maven"} | |||
| KAFKA_CONNECT_PLUGINS_DIR=${$KAFKA_CONNECT_PLUGINS_DIR} | |||
There was a problem hiding this comment.
Should here be really twodollar signs?
There was a problem hiding this comment.
What about the case where I want to install to another dir than KAFKA_CONNECT_PLUGINS_DIR but without moving KAFKA_CONNECT_PLUGINS_DIR? I think that is/was the main reason for having it as separate env vars?
There was a problem hiding this comment.
Should here be really twodollar signs?
No. Fixed!
What about the case where I want to install to another dir than KAFKA_CONNECT_PLUGINS_DIR but without moving KAFKA_CONNECT_PLUGINS_DIR? I think that is/was the main reason for having it as separate env vars?
IMO, the destination directory should be an argument rather than being inferred from the dependency type. This will make this script simpler and it will be easier to follow where libs are downloaded in the docker images.
razvanz
changed the title
|
@Naros and @jpechane I've created a corresponding JIRA issue and connected it with the title of this PR: https://issues.redhat.com/browse/DBZ-7551 |
|
Hi, @razvanz; please prefix all your commits with the DBZ-7551 issue tag. Thanks!. |
By relying on the KAFKA_CONNECT_PLUGINS_DIR in the docker-maven-download.sh (connect-base image), overwriting the MAVEN_DEP_DESTINATION can be avoided in the connect image. This prevents the need to revert this override in decendant images, allowing for a clear distinction between KAFKA_CONNECT_PLUGINS_DIR, MAVEN_DEP_DESTINATION and EXTERNAL_LIBS_DIR.
| @@ -6,7 +6,6 @@ LABEL maintainer="Debezium Community" | |||
| ENV DEBEZIUM_VERSION="2.6.0.Alpha2" \ | |||
| MAVEN_REPO_CENTRAL="" \ | |||
| MAVEN_REPOS_ADDITIONAL="" \ | |||
| MAVEN_DEP_DESTINATION=$KAFKA_CONNECT_PLUGINS_DIR \ | |||
There was a problem hiding this comment.
Sorry, I continue the previous discussion here in this line. In the essence of this PR, aren't we actually talking about this change here? We are now always defaulting to KAFKA_CONNECT_PLUGINS_DIR without any option to install to another destination directory, where previously we could customize.
@jpechane what about special dirs outside KAFKA_CONNECT_PLUGINS_DIR (which are also not lib) things like a scripting package, Apicurio, and other things that sometimes get symlinked into libs or the plugins dir based on env vars? Won't this be affected?
There was a problem hiding this comment.
As every type of dependency is handled by a seperate function it makes sense to remove the common env var and use only the specific ones.
|
@razvanz Applied, thanks. |
|
@jpechane I wonder why this got merged after our recent discussion. Didn't we mean to make the dir configurable by parameter instead of just making the directory fixed to the Kafka plugins dir? Is there a follow-up Jira? |
By relying on the KAFKA_CONNECT_PLUGINS_DIR in the docker-maven-download.sh (connect-base image), overwriting the MAVEN_DEP_DESTINATION can be avoided in the connect image.
This prevents the need to revert this override in decendant images, allowing for a clear distinction between KAFKA_CONNECT_PLUGINS_DIR, MAVEN_DEP_DESTINATION and EXTERNAL_LIBS_DIR.