Conversation
Signed-off-by: Hendrik Leidinger <hendrik.leidinger@nextcloud.com> Assisted-by: Claude Code:Opus 4.8
Signed-off-by: Hendrik Leidinger <hendrik.leidinger@nextcloud.com> Assisted-by: Claude Code:Opus 4.8
Signed-off-by: Hendrik Leidinger <hendrik.leidinger@nextcloud.com> Assisted-by: Claude Code:Opus 4.8
Signed-off-by: Hendrik Leidinger <hendrik.leidinger@nextcloud.com>
Signed-off-by: Hendrik Leidinger <hendrik.leidinger@nextcloud.com>
chrip
left a comment
There was a problem hiding this comment.
Summary
This PR adds a reusable workflow (publish-repo.yml) that pulls the Nextcloud Office APT/DNF archive from the SSH host, merges the new DocumentServer packages in, prunes to two versions, re-signs the indices and pushes the whole tree back with rsync --delete. build.yml uploads the nextcloud-office packages as an artifact and calls the new workflow. The design is sound, and parts of the scripting are careful (atomic mkdir lock, arch read from the RPM header, sort -V pruning). The branch is still in test mode, though, and one failure path can wipe the published archive, so it isn't ready to merge.
Prior feedback
| From | Point | Status |
|---|---|---|
github-advanced-security (zizmor, build.yml) |
actions/upload-artifact@v4 not pinned to a SHA |
🔴 still open |
github-advanced-security (zizmor, publish-repo.yml:105) |
actions/download-artifact@v4 not pinned to a SHA |
🔴 still open |
github-advanced-security (zizmor, build.yml) |
publish-repo job has no permissions: block |
🔴 still open |
github-advanced-security (zizmor, build.yml) |
secrets: inherit passes every secret to the called workflow |
🔴 still open |
Verification
| Claim / Item | Reality | Status |
|---|---|---|
Intended gate contains(github.ref_name, '-stable.') |
No tag in the repo contains -stable.; tags look like v9.3.4, v9.3.4-hotfix.1, v9.3.5-rc.1, v9.3.1-tp.3 |
❌ |
Deb prune glob *_"$arch".deb matches the built filenames |
Built as ${COMPANY}-${PRODUCT}_${VERSION}-${BUILD}_${ARCH}.deb (build/.docker/standalone.bake.Dockerfile:74) |
✓ |
RPMs are signed, so dnf's default gpgcheck=1 works |
They are not; build/vagrant/provision-rpm.sh:97 installs with --nogpgcheck "since the package is locally built and unsigned" |
❌ |
apt-ftparchive release dists/$SUITE > dists/$SUITE/Release produces a clean Release |
Reproduced locally: the output lists Release itself (hash of the partially written file) |
|
*.context=.. is needed for the base targets |
Every target in build/docker-bake.hcl already sets context = ".."; only targets from the private brand-server.hcl could be affected, which I couldn't check |
|
| Mergeable | CONFLICTING: main has since added ${{ github.sha }} to every cache ref in the set: block this PR touches |
❌ |
| DCO / AI disclosure | All five commits signed off; Assisted-by in description and commits |
✓ |
Issues & Suggestions
🔴 Blocking
- Blocking 1: Test-mode gates are still commented out.
build.ymlhasif: matrix.brand.name == 'nextcloud-office' #&& startsWith(github.ref, 'refs/tags/')on the artifact upload and#if: startsWith(github.ref, 'refs/tags/') && contains(github.ref_name, '-stable.')on thepublish-repojob (commits902a6cedandae89c3d7say "for testing"). Merged as is, every push to main, every same-repo PR and everyworkflow_dispatchwould publish its packages into the publicstablesuite. Restoring the commented condition won't fix it either: no existing tag contains-stable., so the job would never run. Please settle which tags count as stable (plainvX.Y.Zplus-hotfix.N? exclude-rc/-tp?) and gate both the upload and the job on that. - Blocking 2: A failed fetch wipes the whole archive. "Fetch current archive" runs
rsync ... "$REMOTE:$REMOTE_BASE/repo/" repo/ || true, which is meant for the first run. The|| truealso swallows a network drop, an auth failure or a partial transfer (rsync exit 23/24).repo/is then empty or incomplete, and "Publish" runsrsync -a --delete repo/ "$REMOTE:$REMOTE_BASE/repo/", deleting everything that wasn't fetched, including the DesktopEditors packages this design exists to preserve. Detect the first run explicitly and let every other failure fail the job:A cheap second guard before "Publish" would help too: refuse to push if the package count dropped compared to the fetch.if $SSH_CMD "$REMOTE" "test -d '$REMOTE_BASE/repo'"; then rsync -a --info=stats2 -e "$SSH_CMD" "$REMOTE:$REMOTE_BASE/repo/" repo/ fi
⚠️ Major
- Major 3: The apt repo is inconsistent while "Publish" runs. rsync walks the tree in sorted order, so under
deb/it writesdists/beforepool/, and insidedists/stable/it writesInReleasebeforemain/binary-*/Packages. During the upload, clients can fetch anInReleasewhose hashes don't match thePackageson the server, and then aPackagesthat points at debs not uploaded yet. With packages of several hundred MB, that window lasts minutes. Push in phases: packages first without--delete, then the indices and signatures, then a final--deletepass to drop pruned files. - Major 4: RPMs are unsigned, so dnf's default
gpgcheck=1rejects them. "Build rpm indices" only signsrepomd.xml. The packages come out of the build unsigned (see Verification), so a user with a stock.repofile gets a signature error on install. Either sign each RPM with the same key (rpm --addsignwith%_gpg_nameset to$KEYID) beforecreaterepo_c, or document that the.repofile needsrepo_gpgcheck=1withgpgcheck=0. I'd go with signing. - Major 5: The zizmor findings listed under Prior feedback are still open. Pin
actions/upload-artifactandactions/download-artifactto SHAs with a version comment, asbuild.yml:508already does foractions/upload-artifact@043fb46d... # v7. Give thepublish-repojob apermissions:block; the workflow needs nothing beyond the artifact download, socontents: readis enough. Replacesecrets: inheritwith the six secretspublish-repo.ymlactually reads (NEXTCLOUD_SSH_KEY,NEXTCLOUD_SSH_HOST,NEXTCLOUD_SSH_USER,NEXTCLOUD_SSH_PORT,NEXTCLOUD_SSH_REMOTE_PATH,NEXTCLOUD_GPG_PRIVATE_KEY), declared underon.workflow_call.secrets. Right now the called workflow also receivesEURO_OFFICE_MIRROR_TOKENand every other repo secret.
ℹ️ Minor / 💡 Suggestions
- Minor 6: Rebase onto main. The
set:hunk conflicts with main's new${{ github.sha }}-scoped cache refs. - Minor 7:
*.context=..is unrelated to repo publishing. It applies to every bake target of both brands, and all targets inbuild/docker-bake.hclalready use"..", so it can only change something inbrand-server.hcl. If the brand repo layout change needs it, say so in the commit message, or split it into its own PR so it doesn't get reverted along with this one. - Minor 8: The SSH upload path moved.
TARGETchanges from$REMOTE_BASE/$PRODUCT_VERSION/$BUILD_NUMBERto$REMOTE_BASE/docserver/.... Anything that downloads from the old location breaks on the next tag, so please mention it in the description. - Minor 9: The Release file lists itself.
> "dists/$SUITE/Release"creates the file beforeapt-ftparchivescans the directory, so it hashes its own partial output (I reproduced this locally). Therm -fabove it doesn't prevent that. Write to a temp file outsidedists/andmvit in. - Minor 10: Lock and concurrency timing. The stale-lock cutoff is 60 minutes but waiters give up after 30, so one cancelled run makes every run in the next half hour fail instead of waiting. A slow run (full archive down and up) that passes 60 minutes can have its lock removed by a waiter. Separately,
concurrencykeeps only one pending run and cancels older pending ones, so back-to-back tags can skip publishing the middle one. That's probably fine withkeep_versions: 2, but it deserves a comment. - Minor 11: Cleanup before merge. The header comment refers to "the original PR", which reads like leftover chat context. The file has no trailing newline. The "for testing" commits should be squashed or reworded.
- 💡 Suggestion 12: Consider
needs: [build, e2e]forpublish-repo, so a tag that fails e2e never reaches the public repo.
Verdict
Request changes. The test-mode gates (Blocking 1) would publish unreleased builds, and the || true plus --delete path (Blocking 2) can delete the DesktopEditors packages. The mid-sync inconsistency and the unsigned RPMs are what users would run into next.
Assisted-by: ClaudeCode:claude-opus-5-5
Signed-off-by: Hendrik Leidinger <hendrik.leidinger@nextcloud.com> Assisted-by: Claude Code:Opus 4.8
Signed-off-by: Hendrik Leidinger <hendrik.leidinger@nextcloud.com>
Signed-off-by: Hendrik Leidinger <hendrik.leidinger@nextcloud.com>
Adds apt/dnf repos for NC Office. Since they will also contain desktop editors, this workflow downloads the current repo, and creates a new repo with new documentserver and desktopeditors from the old repo.
Assisted-by: Claude Code:Opus 4.8