deepakpanda93 commented on code in PR #19489:
URL: https://github.com/apache/hudi/pull/19489#discussion_r3878840545
##########
docker/README.md:
##########
@@ -125,10 +127,28 @@ After new images are built, you can run the following
script to bring up docker
./setup_demo.sh dev
```
-## Upload Updated Image to Repository on Docker Hub
+## Publishing Images to Docker Hub
+
+Publishing is a release action for Hudi maintainers, not part of the normal
build or demo flow. It overwrites the
+images that every `setup_demo.sh` user pulls, so it needs write access to the
`apachehudi/...` Docker Hub
+repositories and a prior `docker login`. If you are contributing a Dockerfile
change, build without `--publish true` and
+let a maintainer publish.
-Once you have built the updated image locally, you can push the corresponding
this repository of the image to the Docker
-Hud registry designated by its name or tag:
+`build_and_publish_docker_images.sh` publishes the whole image set it just
built when given `--publish true`:
+
+```shell
+# Build the image set and publish it
+./build_and_publish_docker_images.sh --publish true
+```
+
+Each image is pushed under both the `latest` tag and the version tag, the
latter taken from the root `pom.xml`
+unless `VERSION_TAG` overrides it. The script reports a per-tag summary and
exits non-zero if any push fails, so a
Review Comment:
You are right, and I took the first option — added the `exit`, rather than
softening the sentence, because the documented behaviour is the one worth
having when publishing overwrites shared repositories.
Confirmed the mechanism before changing anything: `push_images()` ended on
the `if/else` echo with no `return`/`exit`, each `docker push` sits inside an
`if` so `set -e` skips it, and the missing-image branch only increments
`FAILURE_COUNT` and `continue`s. The function therefore returned 0, and the
script's last statement is the `if [ "$PUBLISH" = true ]` block, so the whole
run exited 0.
Fix:
```sh
else
# Fail the script. Publishing overwrites the shared apachehudi/...
repositories, so a partial
# push has to be distinguishable from a complete one by exit code alone:
each docker push runs
# inside an if, which exempts it from set -e, so without this the script
would report success
# after leaving the registry with some images at the new tag and some
stale.
echo "Some pushes failed. Review logs above."
return 1
fi
```
`push_images` is called as a plain command, so `set -e` turns that non-zero
return into a non-zero script exit.
Verified by extracting the real function into a harness with a stubbed
`docker`, rather than only reading it. The partial case is the one you called
out:
| scenario | before | after |
| --- | --- | --- |
| every push fails | 0 | **1** |
| partial publish (2 pushed, 2 failed) | **0** | **1** |
| all pushes succeed | 0 | 0 |
The "before" column is the current code with the `return 1` removed, so it
reproduces the behaviour you described. The README sentence now matches the
script, so no doc change was needed.
Two other things in this push, both from the rebase onto latest master:
- `bcaf2d48e6b7` modified `build_docker_images.sh`, the script this PR
deletes, so git raised a modify/delete conflict. Resolving it as a plain delete
would have dropped that work, so I ported it into
`build_and_publish_docker_images.sh`: the `SPARK_VERSION` validation and the
`hadoop-aws`/`aws-sdk`/`analyticsaccelerator-s3` selection keyed on the Spark
major.minor, plus the three `--build-arg`s on both the buildx and single-arch
paths. Worth being explicit that this was a silent failure mode rather than a
build break: `spark_base/Dockerfile` defaults those args to the Spark 3 pairing
(`3.3.4` / SDK v1), so without them a Spark 4 image would have built
successfully with the wrong S3A jars.
- The README's S3A paragraph came from master naming the removed script; it
now names the new one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
