Skip to content

ci: Enable nc-34#1106

Draft
cbcoutinho wants to merge 8 commits into
masterfrom
feature/nc-34
Draft

ci: Enable nc-34#1106
cbcoutinho wants to merge 8 commits into
masterfrom
feature/nc-34

Conversation

@cbcoutinho

Copy link
Copy Markdown
Owner

No description provided.

@claude

claude Bot commented Jul 18, 2026

Copy link
Copy Markdown

Claude finished @cbcoutinho's task in 1m 53s —— View job


Review: ci: Enable nc-34 (final state, commit abfbc2f)

  • Gather context (diff, comment history, CI state)
  • Review the workflow diff against CLAUDE.md conventions
  • Post findings

The extensive back-and-forth in this thread already covered the substantive investigation (Astrolabe's max-version="33" bound, the fork-build probe, the blast-radius fix, and the deliberate revert). Reviewing the diff as it stands now against origin/master:

.github/workflows/test.yml | 17 +++++++++++++++--
1 file changed, 13 insertions(+), 4 deletions(-)

Confirmed: this is exactly what the thread says it is.

  • nextcloud_version matrix axis: ["32", "33", "34"] — the # - "34" line is uncommented (line 63).
  • The include entry for nextcloud_version: "34" is restored with the same nextcloud:34.0.1@sha256:e15c14f8… pin used in the earlier passing run (64 hex chars, correctly formed — matches the shape of the 32/33 pins).
  • The new comment block (lines 79-88) documents why the lanes are expected to fail and gives a copy-pasteable one-liner to check app-store availability — good practice for a "this will go red on purpose" change, since a future reader hitting red nc34 lanes without that context would reasonably assume regression rather than a known upstream gap.
  • No trace remains of the temporary scaffolding (docker-compose.astrolabe-dev.yml, compose-files override, step re-gating, submodule repoint) — git diff --check is clean, and the changed-file list is just this one file, confirming the round-4/5 revert fully landed.
  • fail-fast: false is set on integration-test (test.yml:59), so the six nc34 failures won't cancel the 32/33 lanes — no blast radius from this change.

No new findings. This is a workflow-only, no-app-code change; ruff/ty/unit tests don't apply, and correctness is only observable via the integration matrix itself, which the author already verified in both directions (green against the fork build, red-as-expected against the published app store).

Restating what's already tracked (Deck card #684), not new asks:

  • Two empty re-trigger commits (9bd7e4c, ba7acff) are still in history — worth squashing before this leaves draft, per the author's own plan.
  • The PR is intentionally in a "will show red nc34 checks" state pending the Astrolabe app-store release — not a CI misconfiguration, just not mergeable yet.

Nothing to flag; the implementation matches its own stated intent.

Branch: feature/nc-34

Uncommenting only the `include:` entry that supplies `nextcloud_image` for
NC 34 did not add any NC-34 coverage: GitHub Actions merges an `include`
entry into existing combinations only when its keys match one, and "34"
was still absent from the base `nextcloud_version` list. Instead it created
a standalone job with no `mode`/`profile`/`markers`, which failed running
`docker compose --profile ` with an empty profile.

Adding "34" to the base axis crosses it with the six `mode` lanes as
intended.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
@cbcoutinho

Copy link
Copy Markdown
Owner Author

Round-1 findings addressed in 5d873bfc:

  • 🔴 nextcloud_version: "34" still commented out in the base matrix: correct, and CI confirmed it — the orphan job integration ( / nc34) failed running docker compose --profile with an empty profile, exactly as predicted. Added - "34" to the base nextcloud_version axis so it crosses with all six mode lanes.

Other observations:

  • nextcloud_image digest pin: no change needed — renovate.json already has a nextcloud-34 matchDepNames entry alongside 32/33, so the pin is managed.
  • Upstream app compatibility: this is precisely what the PR is verifying. The removed Disabled until all upstream apps support NC 34 comment was a placeholder from when NC 34 was too new; the six nc34 lanes now scheduled on this run are the actual check. If any lane fails on an upstream app, that's a real blocker and I'll report it rather than merge.
  • Test coverage: workflow-only change, no application code — no unit/contract tests apply. Correctness is observable only in this run's integration (* / nc34) jobs.

yq parses the workflow and the matrix axis now reads ["32", "33", "34"].

The first NC-34 matrix run showed four of six nc34 lanes failing at
container start, all on the same cause:

    Error: App "Astrolabe" cannot be installed because it is not
    compatible with this version of the server.

Astrolabe's appinfo declared max-version="33", and the app store has no
NC-34 release (GET /api/v1/platform/34.0.0/apps.json omits astrolabe),
so the app-install hook aborts. This is the blocker the removed
"Disabled until all upstream apps support NC 34" comment referred to.

Point the submodule at cbcoutinho/astrolabe#242 (max-version="34") and
enable the dev mount so CI exercises that build instead of the published
app. PHP/Node/build are ungated from `mode != single-user` because the
mount applies to every mode and vendor/ + js/ are not committed.

All three changes are TEMPORARY and must be reverted once astrolabe
publishes an NC-34 release; the PR stays in draft until then.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
@cbcoutinho
cbcoutinho marked this pull request as draft July 18, 2026 13:46
@cbcoutinho

Copy link
Copy Markdown
Owner Author

Root cause of the nc34 failures: Astrolabe declares max-version="33"

The first full NC-34 matrix run failed in four of six lanes (single-user, multi-user-basic, ldap, login-flow-ldap), all at container start with the same error from the app-install hook:

astrolabe app not found, installing from app store...
Error: App "Astrolabe" cannot be installed because it is not compatible with this version of the server.
==> Failed at executing script "/docker-entrypoint-hooks.d/post-installation/20-install-astrolabe-app.sh". Exit code: 1

Confirmed independently against the app store — there is no NC-34 release of Astrolabe:

GET /api/v1/platform/33.0.0/apps.json  -> astrolabe present (0.36.0)
GET /api/v1/platform/34.0.0/apps.json  -> astrolabe absent

Source: appinfo/info.xml declares <nextcloud min-version="32" max-version="33"/>. So the Disabled until all upstream apps support NC 34 comment this PR removed was accurate and still is — the blocker was substantive, not a technicality. (The image digest pin is fine; docker manifest inspect on 34.0.1@sha256:e15c… resolves.)

What this push does

Probes NC 34 against an Astrolabe build that declares compatibility, via cbcoutinho/astrolabe#242 (max-version="34", no code changes):

  • submodule third_party/astrolabe → that branch (also moves off the stale 0.33.1 pin to current main, 0.36.0);
  • dev mount ./third_party/astrolabe:/opt/apps/astrolabe:ro enabled in docker-compose.yml, so the hook symlinks the local build instead of hitting the app store;
  • Set up PHP / Set up Node.js / Build Astrolabe app ungated from mode != 'single-user' — the mount applies to every mode, and vendor/ + js/ aren't committed, so single-user would otherwise symlink an unbuilt app into custom_apps.

Not mergeable in this state — moved to draft

All three changes are temporary. Per CLAUDE.md the dev mount must be re-commented before merge; leaving it on silently pins CI to the local submodule rather than the published app. Merge path:

  1. this run shows the six nc34 lanes green against the compat bump;
  2. astrolabe#242 merges and ships a release declaring max-version="34";
  3. revert the mount + submodule pointer + step gating here, leaving only the matrix axis change, and re-run against the published app.

If the lanes surface genuine NC-34 breakage rather than just the version gate, the fix belongs in the astrolabe repo and #242 grows to cover it — the version bound alone isn't evidence the app works on 34.

Enabling the mount in docker-compose.yml applied it to every job that
brings up the stack, not just the integration matrix. Two jobs that never
build the submodule broke on it: `integration (docling)` and the pact
`Provider verification (astrolabe -> mcp)`. Both failed in `docker compose
up` — an unbuilt checkout has no vendor/ or js/, so the hook symlinks it
into custom_apps and `occ app:enable` aborts.

Move the mount into docker-compose.astrolabe-dev.yml and apply it as an
override via a per-version `compose-files` matrix property, so only the
nc34 lanes overlay the submodule. Also addresses the reviewer's
blast-radius finding: NC 32/33 go back to testing the published app-store
Astrolabe, which is their real production configuration, instead of the
unpublished fork.

PHP/Node/build are re-gated to `mode != 'single-user' || version == '34'`
— needed only where the override is actually applied.

Verified with `docker compose config`: the astrolabe bind mount is present
with the override and absent without it.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
@cbcoutinho

Copy link
Copy Markdown
Owner Author

Round-3 findings addressed in 51ca8a3e — plus two CI failures the round-3 push introduced.

All six nc34 lanes passed on 32ef80e1 against the compat bump (single-user, multi-user-basic, login-flow, keycloak, ldap, login-flow-ldap). The max-version="33" bound was the only obstacle.

🟡 Blast radius — fixed rather than accepted. You were right that the mount wasn't scoped to nextcloud_version == '34', and it turned out to be more than a trade-off: enabling it in docker-compose.yml applied it to every job that brings up the stack, breaking two that never build the submodule — integration (docling) and Provider verification (astrolabe -> mcp). Both died in docker compose up, because an unbuilt checkout has no vendor/ or js/, so the hook symlinks it into custom_apps and occ app:enable aborts. Both were green on the prior run, so 32ef80e1 caused them.

Fix: moved the mount to docker-compose.astrolabe-dev.yml, applied as an override through a per-version compose-files matrix property. Only nc34 overlays the submodule; 32/33 are back on the published app-store Astrolabe (their real production configuration), and docling/pact keep using the base file untouched. Verified with docker compose config — the bind mount is present with the override and absent without it.

PHP/Node/build re-gated to matrix.mode != 'single-user' || matrix.nextcloud_version == '34', so they run only where the override actually applies.

🟢 Revert checklist — tracked, and now smaller: delete docker-compose.astrolabe-dev.yml, drop the compose-files override from the nc34 include entry, drop the || version == '34' clause from the three step conditions, and repoint the submodule at a tagged release. Recorded on Deck card #684 as explicit acceptance criteria, including a second green run against the published app.

🟢 Force-push/GC risk on the SHA pin — acknowledged, accepted for the draft window. b702617 is the tip of cbcoutinho/astrolabe#242 and won't be force-pushed; the pin moves to a release tag before this leaves draft.

Worth stating plainly: green nc34 lanes prove the version gate was the only blocker — not that Astrolabe is broadly sound on NC 34. That distinction is on the card too.

The push of 51ca8a3 created no GitHub Actions runs at all — not queued,
not failed, absent for all three workflows (Tests, Pact, Claude review),
while SonarCloud analyzed the commit normally. Actions is enabled, all
three workflows are active, none filter drafts, the repo is public (so
no minutes limit), and githubstatus reported no incident. Most likely a
dropped pull_request event.

Empty commit to emit a fresh synchronize.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
@cbcoutinho
cbcoutinho marked this pull request as ready for review July 18, 2026 16:15
cbcoutinho and others added 2 commits July 18, 2026 18:17
Pushes made while the PR was a draft produced no Actions runs at all,
though Actions was healthy repo-wide. Marking the PR ready does not by
itself trigger these workflows — ready_for_review is not among their
event types (opened, synchronize, reopened) — so this empty commit
emits a synchronize while the PR is non-draft.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
# Conflicts:
#	.github/workflows/test.yml
@cbcoutinho
cbcoutinho marked this pull request as draft July 18, 2026 16:27
@cbcoutinho

Copy link
Copy Markdown
Owner Author

Round-4: no blocking findings, and CI is green — 29/29 checks (can-i-deploy skipped, shadow mode per ADR-029).

The two jobs the previous push broke both recovered, which was the open question:

  • integration (docling) — SUCCESS
  • Provider verification (astrolabe -> mcp) — SUCCESS

All 18 integration lanes pass, including all six nc34, now also against master's refreshed nc32 digest.

Note on the CI blackout between 51ca8a3e and 9c19d6e7. Pushes in that window produced no Actions runs at all — no queued, no failed, nothing — while SonarCloud analyzed each commit normally. Cause: the PR had become CONFLICTING/DIRTY, and GitHub cannot compute the merge ref for a conflicting PR, so it creates no pull_request runs and surfaces no conflict indicator among the checks. The conflict was self-inflicted: master's 0df29dfb (renovate, nc32 -> 32.0.12) changed the nc32 digest line, and 51ca8a3e added compose-files: immediately after it. Resolved by merging origin/master (took master's digest, kept the override wiring). Worth remembering that gh pr view --json mergeable is the first thing to check when workflows silently stop firing.

Minor observations, both left as-is:

  • Trailing newline from the compose-files: | block literal — real, harmless, and all six nc34 lanes pass with this exact config. Not worth churning a working matrix value for a cosmetic nit.
  • Submodule pinned to a branch tip rather than a release tag — accepted for the draft window; repointing to a tag is on the pre-merge checklist.

Still deliberately not mergeable, independent of the green run. Pending: astrolabe#242 merged and released, then revert the temporary scaffolding (delete docker-compose.astrolabe-dev.yml, drop the compose-files override and the || version == '34' clauses, repoint the submodule), squash out the two empty re-trigger commits, and re-run against the published app. Tracked on Deck card #684.

To restate one thing plainly: green nc34 lanes prove the max-version="33" gate was the only blocker. They are not evidence that Astrolabe is broadly sound on NC 34.

cbcoutinho/astrolabe#242 (max-version="34") is merged, but merging is not
releasing — the app store still returns no astrolabe entry for NC 34:

    curl -sL https://apps.nextcloud.com/api/v1/platform/34.0.0/apps.json \
      | jq '[.[] | select(.id=="astrolabe")] | length'   # => 0

Remove the temporary scaffolding that let CI test NC 34 against the
unpublished submodule build: delete docker-compose.astrolabe-dev.yml,
drop the `compose-files` matrix property, restore the plain compose file
and the `mode != 'single-user'` gating, and repoint the submodule at the
pin master already carries.

The nc34 lanes are now EXPECTED TO FAIL on the app-install hook until the
release is live. That is the intent: the matrix should reflect real
published-app compatibility rather than a local build, so nothing merges
on the strength of a fork that users cannot install.

The diff against master is now purely the matrix axis change plus a note
recording how to check app-store availability.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
@cbcoutinho

Copy link
Copy Markdown
Owner Author

Temporary scaffolding reverted — nc34 lanes now expected to FAIL

cbcoutinho/astrolabe#242 is merged (c7d2e008, 16:49Z), but merging is not releasing. The app store still has no NC-34 build:

$ curl -sL https://apps.nextcloud.com/api/v1/platform/34.0.0/apps.json | jq '[.[] | select(.id=="astrolabe")] | length'
0

So abfbc2fe removes everything that let CI test NC 34 against the unpublished submodule build:

  • deleted docker-compose.astrolabe-dev.yml
  • dropped the compose-files matrix property; compose-file is back to plain ./docker-compose.yml
  • re-gated Set up PHP / Set up Node.js / Build Astrolabe app to matrix.mode != 'single-user'
  • repointed third_party/astrolabe at the pin master already carries (cbab6a7)

The diff against master is now purely the matrix axis change, plus a comment recording the blocker and the one-liner to check app-store availability.

The six integration (* / nc34) lanes will now fail on the app-install hook with App "Astrolabe" cannot be installed because it is not compatible with this version of the server. That is deliberate — the matrix should reflect what users can actually install, so this can't merge on the strength of a fork build.

⏳ Check back when the Astrolabe release is live

Re-check with:

curl -sL https://apps.nextcloud.com/api/v1/platform/34.0.0/apps.json \
  | jq '[.[] | select(.id=="astrolabe")] | length'

Non-zero ⇒ the release is published. Then:

  1. Re-run this PR's CI (a fresh push or re-run of the Tests workflow) and confirm all six nc34 lanes go green against the published app — this is the verification that was previously only done against the fork.
  2. Squash out the two empty re-trigger commits (9bd7e4cb, ba7acff2) from the CI-outage debugging.
  3. Mark ready for review and merge.

Tracked on Deck card #684. For the record: green nc34 lanes on the fork build proved the max-version gate was the only blocker — the published-app run is what actually gates merge.

@sonarqubecloud

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant