royteeuwen commented on code in PR #41: URL: https://github.com/apache/sling-org-apache-sling-committer-cli/pull/41#discussion_r3740762718
########## .github/workflows/docker-push.yml: ########## @@ -0,0 +1,48 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +name: Push Sling Committer CLI Docker image + +on: + push: + branches: + - master + +jobs: + sling-committer-cli-push-image: + runs-on: ubuntu-latest + + steps: + - uses: actions/checkout@v7 + - name: Set up JDK 21 + uses: actions/setup-java@v5 + with: + distribution: 'adopt' Review Comment: `adopt` is deprecated: setup-java emits a deprecation warning for it, and v6 removes the legacy AdoptOpenJDK distributions entirely. It does still resolve a JDK 21 today, so this works — but `temurin` is the drop-in replacement ########## .github/workflows/docker-push.yml: ########## @@ -0,0 +1,48 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +name: Push Sling Committer CLI Docker image + +on: + push: + branches: + - master + +jobs: + sling-committer-cli-push-image: + runs-on: ubuntu-latest + + steps: + - uses: actions/checkout@v7 + - name: Set up JDK 21 + uses: actions/setup-java@v5 + with: + distribution: 'adopt' + java-version: '21' + cache: 'maven' + - name: Login to Docker Hub + id: login-docker-hub + uses: docker/login-action@af1e73f918a031802d376d3c8bbc3fe56130a9b0 + with: + username: ${{ secrets.DOCKERHUB_USER }} + password: ${{ secrets.DOCKERHUB_TOKEN }} + - name: Set up QEMU # https://github.com/docker/buildx/issues/499 + uses: docker/setup-qemu-action@ce360397dd3f832beb865e1373c09c0e9f86d70a + - name: Set up Docker Buildx + uses: docker/setup-buildx-action@4d04d5d9486b7bd6fa91e7baf45bbb4f8b9deedd + - name: Push Docker image + run: mvn --batch-mode clean install -Ddocker.platforms=linux/amd64,linux/arm64 -Dmaven.test.skip=true -Ddocker.skip.push=false -Ddocker.image.tag=latest Review Comment: Two things beyond the push not firing (see the pom comments). **`-Dmaven.test.skip=true` publishes untested code. I'd either run the tests here or gate the push on the Jenkins check. Small one: `-Ddocker.image.tag=latest` is redundant with the pom default ########## pom.xml: ########## @@ -318,12 +323,17 @@ <configuration> <images> <image> - <name>apache/sling-cli</name> + <name>${docker.image.name}:${docker.image.tag}</name> <build> <contextDir>${project.basedir}</contextDir> <args> <FEATURE_FILE>target/artifacts/org/apache/sling/${project.artifactId}/${project.version}/${project.artifactId}-${project.version}-app.slingfeature</FEATURE_FILE> </args> + <buildx> + <platforms> + <platform>${docker.platforms}</platform> Review Comment: Two notes on the platform handling. The empty default is clean — `BuildXConfiguration.getPlatforms()` runs `splitAtCommasAndTrim`, so this element resolves to an empty list, `isBuildX()` returns false, and a local build takes the classic non-buildx path. Verified with `mvn clean install -Dmaven.test.skip=true`: BUILD SUCCESS, `Built image sha256:...` for `apache/sling-committer-cli:latest`. That same split is also why passing a comma-joined value into this single element works in CI. **However, multi-arch will not happen while only the `build` goal runs.** dmp only builds multiple platforms on push, by design: `BuildXService.buildAndLoadSinglePlatform` builds the native platform only (`--load`) whenever the platform list contains the native one, and the real multi-platform build lives in `pushMultiPlatform` (`--push`). So on an amd64 runner with `build` alone, you get an amd64-only image loaded into the runner's daemon and discarded when the job ends — arm64 is never built. Same root cause as the `<goal>push</goal>` comment; fixing the phase fixes both. Note it will then build amd64 twice (the second from cache) before pushing the manifest list. ########## .github/workflows/docker-push.yml: ########## @@ -0,0 +1,48 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +name: Push Sling Committer CLI Docker image + +on: Review Comment: Three additions worth making at this level: - `permissions: contents: read` — this job handles Docker Hub secrets but runs with the default token scope. `allowlist-check.yml` already declares this; good to be consistent. - `concurrency:` group — two master pushes in quick succession race on `latest`, and whichever job finishes last wins regardless of which commit is newer. - `workflow_dispatch:` — without it, re-pushing an image requires an empty commit to master. ########## pom.xml: ########## @@ -333,6 +343,7 @@ <id>default</id> <goals> <goal>build</goal> + <goal>push</goal> Review Comment: **Blocker: this goal never runs, so nothing is pushed.** The execution has no explicit `<phase>`, so each goal falls back to its own mojo default. In docker-maven-plugin 0.48.1 those are `build -> install` and `push -> deploy`. The workflow runs `mvn clean install`, which stops before `deploy`. I ran `mvn -X -B clean install` on this branch, and the project build plan contains exactly one docker goal: ``` [DEBUG] Goal: io.fabric8:docker-maven-plugin:0.48.1:build (default) [INFO] --- docker:0.48.1:build (default) @ sling-cli --- ``` So `-Ddocker.skip.push=false` in the workflow is inert, and the login / QEMU / buildx steps have no effect. The job goes green and Docker Hub stays empty. Fix is to bind the execution explicitly: ```xml <execution> <id>default</id> <phase>install</phase> <goals> <goal>build</goal> <goal>push</goal> </goals> </execution> ``` The `docker.skip.push=true` default you added is exactly what makes this safe for a local `mvn install`. -- 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]
