Skip to content

feat: apt/dnf repo support (NC Office) - #336

Open
rikled wants to merge 8 commits into
mainfrom
feat/pkgrepo
Open

rikled wants to merge 8 commits into
mainfrom
feat/pkgrepo

Conversation

@rikled

@rikled rikled commented Aug 24, 2026 •

Copy link
Copy Markdown
Member

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

Signed-off-by: Hendrik Leidinger <hendrik.leidinger@nextcloud.com>
Assisted-by: Claude Code:Opus 4.8
Comment thread .github/workflows/build.yml Fixed
Comment thread .github/workflows/build.yml Fixed
Comment thread .github/workflows/build.yml Fixed
Signed-off-by: Hendrik Leidinger <hendrik.leidinger@nextcloud.com>
Assisted-by: Claude Code:Opus 4.8
Comment thread .github/workflows/build.yml Fixed
Signed-off-by: Hendrik Leidinger <hendrik.leidinger@nextcloud.com>
Assisted-by: Claude Code:Opus 4.8
Comment thread .github/workflows/publish-repo.yml Fixed
Comment thread .github/workflows/build.yml Fixed
Comment thread .github/workflows/build.yml Fixed
Signed-off-by: Hendrik Leidinger <hendrik.leidinger@nextcloud.com>
Signed-off-by: Hendrik Leidinger <hendrik.leidinger@nextcloud.com>
@rikled
rikled marked this pull request as ready for review September 28, 2026 11:10
@rikled
rikled requested a review from a team as a code owner September 28, 2026 11:10
@rikled
rikled requested review from chrip and removed request for a team September 28, 2026 11:10

@chrip chrip left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.yml has if: 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 the publish-repo job (commits 902a6ced and ae89c3d7 say "for testing"). Merged as is, every push to main, every same-repo PR and every workflow_dispatch would publish its packages into the public stable suite. 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 (plain vX.Y.Z plus -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 || true also swallows a network drop, an auth failure or a partial transfer (rsync exit 23/24). repo/ is then empty or incomplete, and "Publish" runs rsync -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:
    if $SSH_CMD "$REMOTE" "test -d '$REMOTE_BASE/repo'"; then
      rsync -a --info=stats2 -e "$SSH_CMD" "$REMOTE:$REMOTE_BASE/repo/" repo/
    fi
    A cheap second guard before "Publish" would help too: refuse to push if the package count dropped compared to the fetch.

⚠️ Major

  • Major 3: The apt repo is inconsistent while "Publish" runs. rsync walks the tree in sorted order, so under deb/ it writes dists/ before pool/, and inside dists/stable/ it writes InRelease before main/binary-*/Packages. During the upload, clients can fetch an InRelease whose hashes don't match the Packages on the server, and then a Packages that 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 --delete pass to drop pruned files.
  • Major 4: RPMs are unsigned, so dnf's default gpgcheck=1 rejects them. "Build rpm indices" only signs repomd.xml. The packages come out of the build unsigned (see Verification), so a user with a stock .repo file gets a signature error on install. Either sign each RPM with the same key (rpm --addsign with %_gpg_name set to $KEYID) before createrepo_c, or document that the .repo file needs repo_gpgcheck=1 with gpgcheck=0. I'd go with signing.
  • Major 5: The zizmor findings listed under Prior feedback are still open. Pin actions/upload-artifact and actions/download-artifact to SHAs with a version comment, as build.yml:508 already does for actions/upload-artifact@043fb46d... # v7. Give the publish-repo job a permissions: block; the workflow needs nothing beyond the artifact download, so contents: read is enough. Replace secrets: inherit with the six secrets publish-repo.yml actually reads (NEXTCLOUD_SSH_KEY, NEXTCLOUD_SSH_HOST, NEXTCLOUD_SSH_USER, NEXTCLOUD_SSH_PORT, NEXTCLOUD_SSH_REMOTE_PATH, NEXTCLOUD_GPG_PRIVATE_KEY), declared under on.workflow_call.secrets. Right now the called workflow also receives EURO_OFFICE_MIRROR_TOKEN and 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 in build/docker-bake.hcl already use "..", so it can only change something in brand-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. TARGET changes from $REMOTE_BASE/$PRODUCT_VERSION/$BUILD_NUMBER to $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 before apt-ftparchive scans the directory, so it hashes its own partial output (I reproduced this locally). The rm -f above it doesn't prevent that. Write to a temp file outside dists/ and mv it 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, concurrency keeps only one pending run and cancels older pending ones, so back-to-back tags can skip publishing the middle one. That's probably fine with keep_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] for publish-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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: 🏗️ In progress

Development

Successfully merging this pull request may close these issues.

3 participants