owncloud: fix CI install test + docker_operation_progress pull/error-handling bugs it exposed#889
Merged
igorpecovnik merged 8 commits intomainfrom Apr 22, 2026
Merged
Conversation
docker_operation_progress ran the actual docker command inside a subshell piped to dialog_gauge. `exit_code=$?` after the pipe only captured dialog_gauge's rc (almost always 0), so any docker run/rm/ rmi/pull failure was reported as success by this function. Callers like module_owncloud install then returned 0 for a container that never started, and the failure surfaced several steps later in a confusing place (the CI test's downstream `status` check). Thread the subshell's true rc past the pipe via a tmp rc_file: each branch writes 1 to it on failure, main shell reads it after the pipe ends. Also emit the captured stderr to the real stderr (not just the dialog msgbox, which is a no-op for --api / non-TTY callers) so CI logs finally show the actual docker error. Fixes silent-failure for all four ops: pull, rm, rmi, run.
github-actions
Bot
added
size/small
igorpecovnik
force-pushed
the
owncloud-pin-image-10.16.1
branch
from
78d7066 to
9a19dd0
Compare
Contributor
WalkthroughThis pull request makes two changes to the OwnCloud module and its tests. The test configuration adds a 10-second sleep delay after installing the module before running subsequent status and purge operations. The module itself changes the Docker image reference from the Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
owncloud/server:latest no longer exists on Docker Hub — ownCloud stopped publishing the floating `latest` tag after pivoting to Infinite Scale (owncloud/ocis). docker pull now fails with "manifest for owncloud/server:latest not found", and the CI owncloud install test has been red as a result. Pin to the last published classic-server release (10.16.1); the module's env-var / port / volume interface is unchanged and the tagged image matches what this module was written for. Fully qualified with docker.io/ so daemons with a non-default default registry still resolve it.
igorpecovnik
force-pushed
the
owncloud-pin-image-10.16.1
branch
from
9a19dd0 to
fccc81e
Compare
github-actions
Bot
added
size/medium
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tools/modules/software/module_owncloud.sh (1)
12-12: Pin looks correct.Fully-qualified
docker.io/owncloud/server:10.16.1aligns with the PR goal and is consumed consistently byinstall,remove,status, andrunpaths below. Note that this tag is on a deprecated image line (ownCloud moved to Infinite Scale /owncloud/ocis); consider tracking a follow-up to migrate the module toowncloud/ocisbefore upstream stops serving the classic server manifests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/modules/software/module_owncloud.sh` at line 12, The pinned image value ["module_owncloud,dockerimage"] = "docker.io/owncloud/server:10.16.1" is correct and consistent; no immediate code change required, but add a follow-up task (e.g., create an issue or TODO) to migrate the module to the new Infinite Scale image namespace (owncloud/ocis) before upstream stops serving the classic server manifests so the install/remove/status/run paths can be updated to use the new image tag.tests/owncloud.conf (1)
10-10: Prefer a readiness poll over a fixedsleep 10.A hardcoded 10-second wait is racy: it may be too short on slow CI runners (causing
statusto fail before the container is up) and wastes time on fast ones. Consider pollingdocker inspect/docker_is_installeduntil the container is running (with a bounded timeout) instead.♻️ Example polling replacement
- sleep 10 + for _ in $(seq 1 30); do + if docker inspect -f '{{.State.Running}}' owncloud 2>/dev/null | grep -q true; then + break + fi + sleep 1 + done🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/owncloud.conf` at line 10, Replace the fixed "sleep 10" wait with a bounded readiness poll that checks the container state instead of sleeping: repeatedly call docker inspect (or the existing docker_is_installed/status check) for the target container until its "State.Running" is true (or the status command succeeds), retrying with a short interval (e.g., 0.5–1s) and a total timeout (e.g., 30–120s); fail with a clear error if the timeout elapses. Ensure the polling loop references the same container identifier used elsewhere in tests/owncloud.conf and logs or returns the inspect output on final failure to aid debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/owncloud.conf`:
- Line 10: Replace the fixed "sleep 10" wait with a bounded readiness poll that
checks the container state instead of sleeping: repeatedly call docker inspect
(or the existing docker_is_installed/status check) for the target container
until its "State.Running" is true (or the status command succeeds), retrying
with a short interval (e.g., 0.5–1s) and a total timeout (e.g., 30–120s); fail
with a clear error if the timeout elapses. Ensure the polling loop references
the same container identifier used elsewhere in tests/owncloud.conf and logs or
returns the inspect output on final failure to aid debugging.
In `@tools/modules/software/module_owncloud.sh`:
- Line 12: The pinned image value ["module_owncloud,dockerimage"] =
"docker.io/owncloud/server:10.16.1" is correct and consistent; no immediate code
change required, but add a follow-up task (e.g., create an issue or TODO) to
migrate the module to the new Infinite Scale image namespace (owncloud/ocis)
before upstream stops serving the classic server manifests so the
install/remove/status/run paths can be updated to use the new image tag.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 645b6b5c-6599-46f1-b066-1ffcda177fb4
📒 Files selected for processing (2)
tests/owncloud.conftools/modules/software/module_owncloud.sh
The pull-progress parser used
select(.status != null) or (.error != null) | if .error then ...
The `or` lives OUTSIDE select, so jq evaluates `select(X) or Y`
as a boolean expression — select filters the stream, then `or`
returns a bool — and the downstream `| if .error then ...` then
tries to index that bool, yielding
jq: error ... Cannot index boolean with string "error"
once per JSON line in the Docker API pull stream. Every pull
whose response stream contained any status/error lines printed
this error and the subshell took the `exit 1` path at the jq-rc
check, making the pull look like it failed even when the bytes
actually landed in the image store.
Move the `or` inside select():
select((.status != null) or (.error != null)) | if .error ...
Smoke-tested against the three line shapes the Docker engine
emits (status-only, progressDetail, error) — all three now parse
cleanly. Bug was latent for any pull whose response happened to
contain parseable lines; surfaced now because owncloud/server:
10.16.1 emits enough progress lines to make the error visible.
docker_get_image_ref uses `docker image ls --format '{{.Repository}}:
{{.Tag}}' | grep <pattern>`. The local image store strips the
docker.io/ registry prefix for Docker Hub images, so an image
pulled as `docker.io/owncloud/server:10.16.1` lists as
`owncloud/server:10.16.1`. The `docker.io/`-prefixed pattern no
longer matches, docker_is_installed returns 1, and the CI test's
`status` check flunks after a successful install.
Docker Hub is the default registry — the unqualified name pulls
identically from every docker daemon that hasn't been reconfigured
with an alternate default, which was the concern the prefix was
trying to address. Drop the prefix.
…params Modern dockerd returns HTTP 400 for POST /images/create?fromImage=owncloud/server:10.16.1 even though `docker pull owncloud/server:10.16.1` from the CLI succeeds against the same daemon. The API expects fromImage and tag as separate query params; embedding the tag in fromImage is what triggers the rejection. Split `target` on the LAST `:` so a registry:port prefix (registry.example.com:5000/repo:tag) stays in fromImage and only the actual tag goes to `tag=`. Default missing-tag to `latest` to match docker CLI behavior.
Docker 29.x ships with MinAPIVersion 1.44 — the daemon rejects
any request pinned to v1.41 with HTTP 400 before it even parses
fromImage/tag. Omit the `/v1.XX/` prefix and let the daemon pick
its own latest (Docker API is backward-compatible within the
MinAPIVersion..APIVersion window).
Repros cleanly against dockerd 29.1.3:
curl --unix-socket /var/run/docker.sock \
-X POST 'http://localhost/v1.41/images/create?fromImage=X&tag=Y'
→ HTTP 400
curl --unix-socket /var/run/docker.sock \
-X POST 'http://localhost/images/create?fromImage=X&tag=Y'
→ HTTP 200
Drops the now-unused `api_version` local. The endpoints we hit
(`/images/create`) are stable API surface, so version-less calls
work across Docker engine 1.41 .. 29.x inclusive.
docker_operation_progress run already blocks on wait_for_container_ready (20 × 3s) before returning, so the container is guaranteed to be in `running` state (or the readiness timeout has expired) by the time install returns. The explicit sleep between install and status was redundant.
igorpecovnik
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The CI
Owncloud installtest has been red becauseowncloud/server:latestno longer exists on Docker Hub — ownCloud stopped publishing the floatinglatesttag after pivoting to Infinite Scale (owncloud/ocis). Fixing the image pin surfaced three latent bugs indocker_operation_progressthat were swallowing failures and masking real errors. This PR fixes all of them.Changes
module_owncloud.sh— pin toowncloud/server:10.16.1.:latestreturnedmanifest unknown.10.16.1is the last realowncloud/servertag; env-var / port / volume interface (OWNCLOUD_TRUSTED_DOMAINS, PUID/PGID,:8080,/config,/mnt/data) unchanged.docker_operation_progress— surface real exit status pastdialog_gauge.Each branch ran the docker command in a subshell piped to
dialog_gauge;exit_code=$?after the pipe only captureddialog_gauge's rc, so any docker failure bubbled up as success. Thread the subshell's true status via a tmprc_file, and echo captured stderr to real stderr (not justdialog_msgbox, which is a no-op for--api/ non-TTY callers).docker_operation_progress pull— fix mis-parenthesised jqselect().Parser was
select(.status != null) or (.error != null) |—orlived outsideselect, yielding a boolean that the downstreamif .error then …couldn't index (Cannot index boolean with string "error"). Moveorinsideselect().docker_operation_progress pull— splitrepo:tagintofromImage+tag.POST /images/create?fromImage=owncloud/server:10.16.1returns HTTP 400 on modern dockerd even thoughdocker pullfrom the CLI works. Canonical API form is separate query params. Split on the last:soregistry:portprefixes survive.docker_operation_progress pull— drop hardcoded/v1.41/API prefix.Docker 29.x has
MinAPIVersion 1.44; the daemon rejects thev1.41pin with HTTP 400 before it even parses fromImage/tag. Omit the version and let the daemon pick its own latest (MinAPIVersion..APIVersion), which is backward-compatible for/images/create.tests/owncloud.conf— drop post-installsleep.Redundant:
docker_operation_progress runalready blocks onwait_for_container_readybefore returning.Test plan
docker pull owncloud/server:10.16.1works (dockerd 29.1.3)Owncloud install (noble/amd64)and(noble/arm64)greenarmbian-config --api module_owncloud installon noble + dockerd 29.1.3 (MinAPIVersion 1.44) pulls and starts the containermodule_owncloud statusreturns 0 afterwardsmodule_owncloud purgecleans up container + image