feat: add GitLab runner provider - #44
Conversation
|
Warning Review limit reached
Next review available in: 22 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (14)
📝 WalkthroughWalkthroughThe change adds GitLab CI/CD support beside GitHub Actions, introduces provider-aware runner lifecycle and UI behavior, hardens credentials and resource ownership, adds deterministic development packaging and safe deployment, and expands repository validation and release protections. ChangesProvider-aware runner fleet
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (8)
tests/linux-checks.Dockerfile (1)
3-3: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftPin the validation image and APT inputs.
FROM ubuntu:24.04uses a mutable tag.apt-get installalso resolves package versions from the live Ubuntu mirror. A later build can use different tool versions or fail after repository changes.Pin the base image by digest. Use a timestamped APT snapshot with explicit package versions if Line 1’s reproducibility claim is required.
Also applies to: 6-19
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/linux-checks.Dockerfile` at line 3, Update the tests/linux-checks.Dockerfile validation image to use an immutable Ubuntu digest instead of the mutable ubuntu:24.04 tag, and configure APT to use a timestamped Ubuntu snapshot with explicit versions for every installed package. Preserve the existing validation packages and commands while ensuring repeated builds resolve identical inputs.install-dev.sh (2)
160-160: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
PACKAGEassignment.
PACKAGEis never read. The tar at line 185 uses$PACKAGE_FILE, and no other reference exists. Shellcheck flags it as SC2034.♻️ Proposed cleanup
[ -n "$PACKAGE_FILE" ] && [ -n "$PACKAGE_SHA256" ] && [ -n "$PLUGIN_SHA256" ] && [ -n "$MANIFEST_SHA256" ] \ || die "bundle validator returned an incomplete result" - PACKAGE="$BUNDLE_DIR/$PACKAGE_FILE" }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@install-dev.sh` at line 160, Remove the unused PACKAGE assignment near the bundle path setup; keep the existing PACKAGE_FILE usage for the tar command unchanged.Source: Linters/SAST tools
277-278: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClean up the temporary baseline and commit directories on failure.
Line 277 creates
$dev_root/.rollback.XXXXXXand line 349 creates$artifacts/.commit.XXXXXX. Everyfailafter those points aborts the remote script and leaves the temporary path on flash. Line 274 only tests$rollback, so the leftovers are never noticed and accumulate across failed runs.Register a cleanup trap for each temporary path, or remove the leftover before
failexits.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@install-dev.sh` around lines 277 - 278, Update the failure handling in the rollback and commit flows to clean up both temporary directories created by the rollback_tmp setup and the corresponding commit-directory creation before fail exits. Register cleanup traps for each path, or otherwise ensure every fail after those creations removes the temporary directory, while preserving the existing rollback check behavior.tests/package-contents.sh (1)
50-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the new GitLab fail-closed branch in the generated remove action.
The required-line list covers the missing-Docker case and the "resources still exist" case. It omits the two enumeration-failure branches that this PR adds in
build-plg.sh(lines 503 and 511):Docker ownership enumeration failedandGitLab executor ownership enumeration failed. Those branches are the fail-closed guard for the new GitLab executor labels, so a regression that drops them passes this test.💚 Proposed additional assertions
'cleanup engine is missing and Docker is unavailable' \ + 'cleanup engine is missing and Docker ownership enumeration failed' \ + 'cleanup engine is missing and GitLab executor ownership enumeration failed' \ 'cleanup engine is missing while plugin-owned Docker resources still exist' \🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/package-contents.sh` around lines 50 - 62, Update the required_remove_line assertions in tests/package-contents.sh to include both fail-closed messages emitted by the generated remove action: “Docker ownership enumeration failed” and “GitLab executor ownership enumeration failed.” Keep the existing required-line checks unchanged.tests/config-parity.sh (1)
45-49: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the duplicated
command -v phptest.Line 45 already records the failure when
phpis missing. The secondcommand -v phpon line 46 only prevents invoking a missing binary, which anelif-style guard expresses more directly.♻️ Proposed simplification
-command -v php >/dev/null 2>&1 || bad "php is required to validate default.cfg" -if command -v php >/dev/null 2>&1 \ - && ! php -r '$v = parse_ini_file($argv[1]); exit(is_array($v) ? 0 : 1);' "$CFG"; then - bad "default.cfg is not valid for PHP parse_ini_file (and therefore Unraid parse_plugin_cfg)" -fi +if ! command -v php >/dev/null 2>&1; then + bad "php is required to validate default.cfg" +elif ! php -r '$v = parse_ini_file($argv[1]); exit(is_array($v) ? 0 : 1);' "$CFG"; then + bad "default.cfg is not valid for PHP parse_ini_file (and therefore Unraid parse_plugin_cfg)" +fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/config-parity.sh` around lines 45 - 49, In the validation flow around the initial PHP prerequisite check and the parse_ini_file command, remove the duplicated command -v php condition and use an elif-style branch so PHP parsing runs only after the prerequisite succeeds. Preserve the existing bad messages and validation behavior.deploy.sh (1)
42-46: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMatch
install-dev.shand allowlist the staging-path suffix.Line 44 checks only the
"$DEST".deploy.prefix.$REMOTE_STAGEis then interpolated into single-quoted remote command strings at lines 49 and 57, so a suffix that contains'would terminate the quoting.install-dev.shlines 177-179 already constrain its suffix to[A-Za-z0-9]. Apply the same check here for consistency.This is hardening, not a privilege gain: the operator already grants the host a root shell.
🛡️ Proposed suffix check
case "$REMOTE_STAGE" in "$DEST".deploy.*) ;; *) echo "deploy: remote host returned an unsafe staging path: $REMOTE_STAGE" >&2; exit 1 ;; esac +case "${REMOTE_STAGE#"$DEST".deploy.}" in + ''|*[!A-Za-z0-9]*) echo "deploy: remote host returned an unsafe staging suffix: $REMOTE_STAGE" >&2; exit 1 ;; +esac🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy.sh` around lines 42 - 46, Strengthen the REMOTE_STAGE validation in deploy.sh by requiring the staging-path suffix after "$DEST".deploy. to contain only the same allowed alphanumeric characters enforced by install-dev.sh. Keep rejecting values with an unexpected prefix, and reject any suffix containing quotes or other non-allowlisted characters before REMOTE_STAGE is used in remote commands.tests/gitlab-runner-lint.sh (1)
23-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the two derived preconditions before using them.
Two values are used without a check:
- Line 23 derives
GITLAB_RUNNER_IMAGEwithsedfromdefault.cfg. If that line's format changes, the variable becomes empty and line 100 runsdocker image inspect "", which reports an unrelated Docker error.- Lines 36-40 assume the host provides
/etc/ssl/certs/ca-certificates.crtor/etc/ssl/cert.pem. If neither exists,cpaborts the test withcp: cannot stat, not a test message.Add explicit checks so a failure names the missing precondition.
♻️ Proposed guards
GITLAB_RUNNER_IMAGE="$(sed -n 's/^GITLAB_RUNNER_IMAGE="\([^"]*\)".*/\1/p' src/usr/local/emhttp/plugins/ci-runner-farm/default.cfg | head -1)" +[ -n "$GITLAB_RUNNER_IMAGE" ] \ + || { echo "gitlab-runner-lint: could not read GITLAB_RUNNER_IMAGE from default.cfg" >&2; exit 1; } @@ if [ -r /etc/ssl/certs/ca-certificates.crt ]; then cp /etc/ssl/certs/ca-certificates.crt "$GITLAB_CA_FILE" +elif [ -r /etc/ssl/cert.pem ]; then + cp /etc/ssl/cert.pem "$GITLAB_CA_FILE" else - cp /etc/ssl/cert.pem "$GITLAB_CA_FILE" + echo "gitlab-runner-lint: no host CA bundle found for the self-managed CA case" >&2 + exit 1 fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/gitlab-runner-lint.sh` around lines 23 - 40, In tests/gitlab-runner-lint.sh, validate that GITLAB_RUNNER_IMAGE extracted from default.cfg is non-empty before any Docker image inspection, and fail with a clear precondition message if it is missing. Also update the CA bundle selection around GITLAB_CA_FILE to check both candidate paths explicitly, reporting a descriptive failure when neither exists instead of attempting cp and exposing only its system error.src/usr/local/emhttp/plugins/ci-runner-farm/include/providers/github.sh (1)
165-166: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBoth adapters build
namefromidxinside the samelocalstatement. Bash expands every word of a declaration command before it creates the locals, so${idx}in the default fornameresolves against the enclosing scope instead of"$1".cmd_recyclecallsbuild_args "$idx"with one argument, which makes the GitHub path reachable and dependent on a dynamically scopedidx. Split the declaration at both sites.
src/usr/local/emhttp/plugins/ci-runner-farm/include/providers/github.sh#L165-L166: declareidxon its own line, then declarenamewith the${NAME_PREFIX}-${idx}default.src/usr/local/emhttp/plugins/ci-runner-farm/include/providers/gitlab.sh#L744-L745: apply the same split ingitlab_build_manager_args.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/usr/local/emhttp/plugins/ci-runner-farm/include/providers/github.sh` around lines 165 - 166, Split the combined local declarations in github_build_args so idx is declared before name, allowing name’s ${NAME_PREFIX}-${idx} default to use the function argument; make the same change in gitlab_build_manager_args at src/usr/local/emhttp/plugins/ci-runner-farm/include/providers/gitlab.sh lines 744-745. No other behavior should change.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@deploy.sh`:
- Around line 102-103: Remove the trailing `|| true` from both `find` commands
that chmod files under `"$stage/nchan"` and `"$stage/event"`, allowing chmod
failures to propagate and fail the deployment. Keep the existing file selection
and permission mode unchanged.
- Around line 261-272: Update the rollback function in deploy.sh to distinguish
signal-triggered execution from ERR-triggered execution: preserve the captured
failure status for ERR, but when invoked by HUP, INT, or TERM, restore the
backup and exit with a non-zero status. Keep the existing rollback and
successful cleanup behavior unchanged.
In `@install-dev.sh`:
- Around line 453-457: Update the legacy mirror verification in install-dev.sh
to match deploy.sh’s provenance checks: validate the ci-runner-mirror
container’s name, image, mount source, and REGISTRY_PROXY_REMOTEURL before
treating it as plugin-owned. Only fail rollback for a matching owned container;
otherwise allow unrelated containers to remain or clearly instruct the operator
to rename them.
In `@src/usr/local/emhttp/plugins/ci-runner-farm/default.github.Dockerfile`:
- Around line 31-39: Update the generated wait-docker.sh script in the
Dockerfile so that after the 90-attempt readiness loop, it performs one final
docker info check and exits nonzero when Docker is still unavailable. Keep exec
"$@" reachable only after successful readiness, allowing the container restart
policy to retry failed startup.
In `@tests/deploy-uninstall-safety.sh`:
- Around line 11-19: Prevent grep-based line lookups from exiting under set
-e/pipefail before diagnostic guards run. In tests/deploy-uninstall-safety.sh
lines 11-19, make first_line tolerate pipeline failures; at lines 128-132, make
runtime_delete_line, package_delete_line, and success_line lookups tolerate
failures and assert all three are nonempty before numeric comparisons. In
tests/install-dev-safety.sh lines 9-14, make line_of tolerate failures. In
tests/provider-mocks.sh lines 1334-1337 and 1186-1187, make stop_line,
unregister_line, job_image_line, and manager_start_line lookups tolerate
failures so existing diagnostics remain reachable.
In `@tests/provider-contract.sh`:
- Around line 190-192: Replace the vacuous Dockerfile assertion in the provider
contract checks with an exact assertion for the legacy unsuffixed GitHub
fallback expression used by the engine. Keep the provider-specific editable and
shipped Dockerfile checks unchanged, and ensure the new condition cannot pass
merely because those strings contain “Dockerfile”.
---
Nitpick comments:
In `@deploy.sh`:
- Around line 42-46: Strengthen the REMOTE_STAGE validation in deploy.sh by
requiring the staging-path suffix after "$DEST".deploy. to contain only the same
allowed alphanumeric characters enforced by install-dev.sh. Keep rejecting
values with an unexpected prefix, and reject any suffix containing quotes or
other non-allowlisted characters before REMOTE_STAGE is used in remote commands.
In `@install-dev.sh`:
- Line 160: Remove the unused PACKAGE assignment near the bundle path setup;
keep the existing PACKAGE_FILE usage for the tar command unchanged.
- Around line 277-278: Update the failure handling in the rollback and commit
flows to clean up both temporary directories created by the rollback_tmp setup
and the corresponding commit-directory creation before fail exits. Register
cleanup traps for each path, or otherwise ensure every fail after those
creations removes the temporary directory, while preserving the existing
rollback check behavior.
In `@src/usr/local/emhttp/plugins/ci-runner-farm/include/providers/github.sh`:
- Around line 165-166: Split the combined local declarations in
github_build_args so idx is declared before name, allowing name’s
${NAME_PREFIX}-${idx} default to use the function argument; make the same change
in gitlab_build_manager_args at
src/usr/local/emhttp/plugins/ci-runner-farm/include/providers/gitlab.sh lines
744-745. No other behavior should change.
In `@tests/config-parity.sh`:
- Around line 45-49: In the validation flow around the initial PHP prerequisite
check and the parse_ini_file command, remove the duplicated command -v php
condition and use an elif-style branch so PHP parsing runs only after the
prerequisite succeeds. Preserve the existing bad messages and validation
behavior.
In `@tests/gitlab-runner-lint.sh`:
- Around line 23-40: In tests/gitlab-runner-lint.sh, validate that
GITLAB_RUNNER_IMAGE extracted from default.cfg is non-empty before any Docker
image inspection, and fail with a clear precondition message if it is missing.
Also update the CA bundle selection around GITLAB_CA_FILE to check both
candidate paths explicitly, reporting a descriptive failure when neither exists
instead of attempting cp and exposing only its system error.
In `@tests/linux-checks.Dockerfile`:
- Line 3: Update the tests/linux-checks.Dockerfile validation image to use an
immutable Ubuntu digest instead of the mutable ubuntu:24.04 tag, and configure
APT to use a timestamped Ubuntu snapshot with explicit versions for every
installed package. Preserve the existing validation packages and commands while
ensuring repeated builds resolve identical inputs.
In `@tests/package-contents.sh`:
- Around line 50-62: Update the required_remove_line assertions in
tests/package-contents.sh to include both fail-closed messages emitted by the
generated remove action: “Docker ownership enumeration failed” and “GitLab
executor ownership enumeration failed.” Keep the existing required-line checks
unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 270eb1f7-2204-40e8-a125-630325e582ab
📒 Files selected for processing (42)
.dockerignore.github/workflows/lint.yml.github/workflows/release-please.yml.github/workflows/release.ymlREADME.mdbuild-plg.shcommunity-applications/DESCRIPTION.mdcommunity-applications/ci-runner-farm.xmldeploy.shinstall-dev.shsrc/usr/local/emhttp/plugins/ci-runner-farm/README.mdsrc/usr/local/emhttp/plugins/ci-runner-farm/RunnerFarmFleet.pagesrc/usr/local/emhttp/plugins/ci-runner-farm/RunnerFarmImage.pagesrc/usr/local/emhttp/plugins/ci-runner-farm/RunnerFarmSettings.pagesrc/usr/local/emhttp/plugins/ci-runner-farm/default.cfgsrc/usr/local/emhttp/plugins/ci-runner-farm/default.github.Dockerfilesrc/usr/local/emhttp/plugins/ci-runner-farm/default.gitlab.Dockerfilesrc/usr/local/emhttp/plugins/ci-runner-farm/event/docker_startedsrc/usr/local/emhttp/plugins/ci-runner-farm/event/stopping_dockersrc/usr/local/emhttp/plugins/ci-runner-farm/include/crf-core.phpsrc/usr/local/emhttp/plugins/ci-runner-farm/include/exec.phpsrc/usr/local/emhttp/plugins/ci-runner-farm/include/providers/github.shsrc/usr/local/emhttp/plugins/ci-runner-farm/include/providers/gitlab.shsrc/usr/local/emhttp/plugins/ci-runner-farm/include/runner-farm.shtests/check.shtests/config-parity.shtests/deploy-uninstall-safety.shtests/dev-package.shtests/exec-csrf.shtests/firewall-transition.shtests/gitlab-policy.shtests/gitlab-runner-lint.shtests/install-dev-safety.shtests/linux-checks.Dockerfiletests/ownership-safety.shtests/package-contents.shtests/provider-contract.shtests/provider-mocks.shtests/release-guard.shtests/resource-ownership.shtests/run-linux-checks.shtests/safe-paths.sh
|
Addressed all actionable review feedback in
Validation completed:
The generic CodeRabbit docstring-coverage notice is advisory rather than an unresolved thread or repository-enforced check; this Bash/PHP plugin has no docstring construct, so adding pseudo-docstrings would be unrelated churn. CodeRabbit currently reports pass. |
|
Thanks so much for this contribution jonschumaker - I'm excited to get this bad boy merged, made you a PR to your repo to fix some potential issues, would love for you to test my fixes before merging and then let me know! |
3 participants
Summary
Add GitLab as a second CI provider while preserving the existing GitHub Actions runner behavior and keeping GitHub as the default.
The implementation consumes the official GitLab Runner image and uses the modern reusable
glrt-authentication-token workflow. Each farm slot gets a persistent runner manager identity and an isolated Docker executor backed by a private, per-slot DinD daemon over a Unix socket.What changed
CI_PROVIDER=github|gitlaband extract provider-specific behavior into GitHub and GitLab adapters.config.tomland.runner_system_idfiles.Why
CI Runner Farm currently manages only GitHub Actions runners. This adds GitLab.com and self-managed GitLab support without forking GitLab Runner itself or duplicating the farm's Docker, cache, locking, scaling, reconciliation, and image-update lifecycle.
The private DinD design reduces exposure compared with mounting Unraid's Docker socket into jobs. It deliberately limits the remaining privileged-Docker blast radius to one single-concurrency farm slot and surfaces that limitation in the UI.
Compatibility and security
glrt-; an API token is optional and is used only for advisory dashboard data./runner-services/docker.sock; the sidecar has no host bindings and no listeners on ports 2375 or 2376.SIGQUITwith a configurable stop timeout.Validation
bash tests/run-linux-checks.shdocker:27-dindvalidation confirmed the private Unix socket works and ports 2375/2376 are not listening.Operational notes
EPHEMERAL,RUN_AS_ROOT, and workspace tmpfs controls remain GitHub-specific because the GitLab Docker executor provides different job-isolation semantics.Summary by CodeRabbit