Skip to content

Conversation

@nodece
Copy link
Member

@nodece nodece commented May 21, 2025

Motivation

The PR #24168 introduced a regression where the pulsar:${project.version}-${git.commit.id.abbrev} Docker image cannot be pulled in the pulsar-all Dockerfile, because we didn't build and push that to the DockerHub.

We consistently pushed both the latest and ${project.version}-${git.commit.id.abbrev} tags to DockerHub in the past. However, the only tag we require is the uniquely identifiable ${project.version}-${git.commit.id.abbrev}. The latest tag can be ignored for our use case.

We recommend using the ${project.version}-${git.commit.id.abbrev} format instead of latest, providing clearer traceability and avoiding ambiguity.

Modifications

  • Removed the custom Docker tag configuration, and -Ddocker.tag=custom-tag is the right path:
     mvn install -pl docker/pulsar,docker/pulsar-all -DskipTests \
      -Pmain,docker,docker-push \
      -Ddocker.platforms=linux/amd64,linux/arm64 \
      -Ddocker.organization=nodece \
      -Ddocker.image=pulsar \
      -Ddocker.tag=pr-24325
    
  • Changed docker.tag value from latest to${project.version}-${git.commit.id.abbrev}.

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

@nodece nodece requested a review from lhotari May 21, 2025 04:47
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label May 21, 2025
@lhotari
Copy link
Member

lhotari commented May 21, 2025

Please check that test image builds are also consistent.

@nodece nodece force-pushed the fix-docker-push branch from 40b11fe to 6da0d35 Compare May 21, 2025 07:50
@nodece
Copy link
Member Author

nodece commented May 21, 2025

@lhotari Done.

@lhotari
Copy link
Member

lhotari commented May 21, 2025

@nodece I remember having some problems with git-commit-id-plugin configuration in the past when someone builds the source code directly without git. Since docker.tag is now set at the top level, it would also be necessary to revisit the git-commit-id-no-git profiles in docker projects and possibly also revisit git-commit-id-plugin configuration.
Please test building the docker image directly from the source package to ensure that it also works.

@nodece
Copy link
Member Author

nodece commented May 21, 2025

@lhotari Both are working fine.

# git env
mvn install -pl docker/pulsar,docker/pulsar-all -DskipTests \
    -Pmain,docker,docker-push \
    -Ddocker.platforms=linux/amd64,linux/arm64 \
    -Ddocker.organization=nodece \
    -Ddocker.image=pulsar

# no .git
# rm -rf .git
mvn install -pl docker/pulsar,docker/pulsar-all -DskipTests \
    -Pmain,docker,docker-push,git-commit-id-no-git \
    -Ddocker.platforms=linux/amd64,linux/arm64 \
    -Ddocker.organization=xxxx \
    -Ddocker.image=pulsar

@lhotari
Copy link
Member

lhotari commented May 21, 2025

@lhotari Both are working fine.

# git env
mvn install -pl docker/pulsar,docker/pulsar-all -DskipTests \
    -Pmain,docker,docker-push \
    -Ddocker.platforms=linux/amd64,linux/arm64 \
    -Ddocker.organization=nodece \
    -Ddocker.image=pulsar

# no-git env
mvn install -pl docker/pulsar,docker/pulsar-all -DskipTests \
    -Pmain,docker,docker-push,git-commit-id-no-git \
    -Ddocker.platforms=linux/amd64,linux/arm64 \
    -Ddocker.organization=xxxx \
    -Ddocker.image=pulsar

What I mean is that the current git-commit-id-no-git profile configuration no longer makes sense when it's in the sub projects. It should automatically get activated at the top level so that it gets properly resolved in the docker.tag property value. I don't think that manually activating the profile would create a proper tag. It's also possible that git-commit-id-no-git profile isn't needed any more and there's a better way to configure the fallback value.

@nodece
Copy link
Member Author

nodece commented May 21, 2025

@lhotari git-commit-id-no-git has been removed. Makes sense.

@lhotari
Copy link
Member

lhotari commented May 21, 2025

@lhotari git-commit-id-no-git has been removed. Makes sense.

What is the default docker.tag in this case? Please test the docker build from the source package so that we don't break that.

@lhotari
Copy link
Member

lhotari commented May 21, 2025

btw. We could consider upgrading the deprecated pl.project13.maven:git-commit-id-plugin to io.github.git-commit-id:git-commit-id-maven-plugin https://github.com/git-commit-id/git-commit-id-maven-plugin.

@lhotari
Copy link
Member

lhotari commented May 21, 2025

btw. We could consider upgrading the deprecated pl.project13.maven:git-commit-id-plugin to io.github.git-commit-id:git-commit-id-maven-plugin https://github.com/git-commit-id/git-commit-id-maven-plugin.

The plugin is Java 11+, so for Maven Shade Java 8 tests to work, the plugin would have to be disabled when running on Java 8 in the existing integration-test-java8 profile.

@liangyepianzhou liangyepianzhou added this to the 4.1.0 milestone May 21, 2025
@nodece
Copy link
Member Author

nodece commented May 21, 2025

@lhotari

What is the default docker.tag in this case?

${project.version}-${git.commit.id.abbrev} is default value.

Please test the docker build from the source package so that we don't break that.

If no .git, I suggest that you append -Ddocker.tag=${project.version}-custom, otherwise, you will get this error:

[ERROR] Failed to execute goal io.fabric8:docker-maven-plugin:0.45.1:build (default) on project pulsar-docker-image: Execution default of goal io.fabric8:docker-maven-plugin:0.45.1:build failed: Given Docker name 'nodece/pulsar:4.1.0-SNAPSHOT-${git.commit.id.abbrev}' is invalid:
[ERROR]    * tag part '4.1.0-SNAPSHOT-${git.commit.id.abbrev}' doesn't match allowed pattern '^[\w][\w.-]{0,127}$'
[ERROR] See http://bit.ly/docker_image_fmt for more details
[ERROR] -> [Help 1]
[ERROR] 
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR] 

Do you have any idea? Or keep git-commit-id-no-git? I prefer -Ddocker.tag.

btw. We could consider upgrading the deprecated pl.project13.maven:git-commit-id-plugin to io.github.git-commit-id:git-commit-id-maven-plugin https://github.com/git-commit-id/git-commit-id-maven-plugin.

Ok, I will open a new PR to upgrade this plugin.

@nodece
Copy link
Member Author

nodece commented May 22, 2025

@lhotari Ping

@nodece
Copy link
Member Author

nodece commented May 26, 2025

Ping @lhotari

@lhotari
Copy link
Member

lhotari commented Jun 3, 2025

Do you have any idea? Or keep git-commit-id-no-git? I prefer -Ddocker.tag.

we could move git-commit-id-no-git profile to the main pom.xml so that things would work also in the case where the build is run for the source distribution. That would be the most obvious solution. The profile should get activated only if docker.tag isn't set.

@nodece nodece force-pushed the fix-docker-push branch from c068fe2 to f0a4704 Compare June 4, 2025 08:00
Signed-off-by: Zixuan Liu <[email protected]>
@nodece
Copy link
Member Author

nodece commented Jun 4, 2025

@lhotari Done.

@coderzc coderzc modified the milestones: 4.1.0, 4.2.0 Sep 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc-not-needed Your PR changes do not impact docs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants