forgejo-runner (FIX): support generated/unattended mode and configurable runner labels#1645
Conversation
|
I recorded each of my tests using https://github.com/asciinema/asciinema and https://github.com/asciinema/agg to render them as gifs after modifying out internal information (storage and template devices, network info, forgejo url and key) as well as removing long waits (OS/container updates, installs, etc). Each test case is shown below. Test 1Test 1: Interactive install: verify prompts appear for instance URL, token, and additional labels
Test 2Test 2: Interactive install with no extra labels: verify only linux-amd64:docker://node:22-bookworm is registered
Test 3Test 3: Interactive install with extra labels: verify default + custom labels are both registered
Test 4 has been withheld at this time, pending question asked in issue #1576 (comment) Test 5Test 5: Unattended install with missing token: verify hard error and clean exit
|
…le runner labels - Export app-specific variables (var_forgejo_instance, var_forgejo_runner_token, var_runner_labels) in ct script so they survive lxc-attach into the container - Replace nonexistent prompt_input_required/show_missing_values_warning calls with standard read prompts that skip when variables are pre-set - Add hard error when no runner token is provided - Make runner labels configurable via var_runner_labels instead of hardcoded - Update default runner image from node:20-bookworm to node:22-bookworm (Node 22 LTS)
CT script hardcoded the upstream URL for build.func, so the install script was always fetched from upstream — ignoring fork/branch fixes. Now respects COMMUNITY_SCRIPTS_URL override, consistent with how build.func already handles this for all other resource fetching.
core.func overwrites $HOSTNAME with a formatted emoji string for display purposes, causing runner registration to send garbled ANSI codes as the runner name. Use $(hostname) to get the actual system hostname.
forgejo-runner register writes .runner config to the current working directory. The install script runs in a temp directory (via build.func), so the config was lost on cleanup. The systemd service expects it in /root (WorkingDirectory=/root), so cd there before registering.
Abort before build_container if mode is set (unattended) but no runner registration token was provided. Avoids a 20+ minute container build only to fail at the registration step.
…active Check for missing token/URL only when mode is set AND there's no TTY, so mode=default with a terminal still allows interactive prompts inside the container.
mode is only set when the user is automating (e.g. mode=default with var_* overrides). The default bash -c curl invocation leaves mode empty and shows the whiptail menu, so this check won't affect interactive use.
WaffleThief123
force-pushed
the
forgejo-runner-fix
branch
from
1b19d62 to
17b2229
Compare
WaffleThief123
changed the title
WaffleThief123
changed the title
|
@lengschder97 could you take a look and review this? @WaffleThief123 can you revert the change at the very top with the URL for sourcing the funcs? |
| if [[ -n "${var_runner_labels:-}" ]]; then | ||
| RUNNER_LABELS="${DEFAULT_RUNNER_LABELS},${var_runner_labels}" | ||
| else | ||
| RUNNER_LABELS="${DEFAULT_RUNNER_LABELS}" | ||
| fi |
There was a problem hiding this comment.
can't you do something like this?
var_keyctl="${var_keyctl:-1}"
There was a problem hiding this comment.
That pattern works for var_forgejo_instance since it has a sensible default (https://codeberg.org), and that's already how ct/forgejo-runner.sh declares it at the top of the file. But it doesn't work for the other two:
-
var_forgejo_runner_tokenhas no sensible default. Every install needs a unique secret from the user's Forgejo instance. The previous code usedREPLACE_WITH_YOUR_TOKENas a fallback, but that just delays failure: the runner registration call rejects it and the install dies mid-way with a confusing auth error. -
var_runner_labelsis opt-in extra labels appended to a default. Using${var_runner_labels:-}silently would skip the interactive prompt entirely for users running the script the normal way (bash -c "$(curl …ct/forgejo-runner.sh)").
The read -r -p block is what gives the interactive UX. Happy to swap to a helper if there's a preferred one. prompt_input_required was the original choice, but it returns a fallback string on timeout or non-TTY rather than failing, which is what landed us with the broken REPLACE_WITH_YOUR_TOKEN behavior.
|
|
||
| msg_info "Registering Forgejo Runner" | ||
| export DOCKER_HOST="unix:///run/podman/podman.sock" | ||
| cd /root |
There was a problem hiding this comment.
forgejo-runner register writes the resulting .runner config file to the current working directory, and the systemd unit we generate a few lines later runs the daemon with WorkingDirectory=/root. Without the cd, the config lands wherever build.func or install.func left CWD (varies by phase, typically not /root), the daemon can't find it on start, and the runner sits idle. cd /root aligns the register step with the service's working dir. We can always change the workdir, but generally if you need to drop into a worker shell, you're likely doing it to modify your .runner config, so keeping it right where the user drops into is my opinionated choice. I am open to a --config /root/.runner flag instead if you prefer that, but I think that adds additional noise.
| --token "$FORGEJO_RUNNER_TOKEN" \ | ||
| --name "$HOSTNAME" \ | ||
| --labels "linux-amd64:docker://node:20-bookworm" \ | ||
| --name "$(hostname)" \ |
There was a problem hiding this comment.
I think hostname var is set by our core funcs, why does this need changing?
There was a problem hiding this comment.
It is set by core funcs, and that's the problem. misc/core.func:114 does this:
HOSTNAME="${TAB}🏠${TAB}${CL}"It's clobbering the bash builtin $HOSTNAME (which Bash normally sets to the system hostname) with a decorative emoji used elsewhere as a label icon. So --name "$HOSTNAME" was registering every runner with literal "🏠" wrapped in tab and color escape codes, which is also why duplicate runner names showed up upon re-running the install. $(hostname) calls the system command and bypasses the shadowed variable.
| msg_ok "Created Services" | ||
|
|
||
| # Show warning if any required values used fallbacks | ||
| show_missing_values_warning |
There was a problem hiding this comment.
That call only fired the warning if MISSING_REQUIRED_VALUES had entries, which only happened when prompt_input_required returned a fallback string. Now that the install hard-fails up top when the token is missing (and we don't accept fake fallbacks like REPLACE_WITH_YOUR_TOKEN anymore), MISSING_REQUIRED_VALUES is always empty by the time we reach that line, so the call is a no-op. I removed it for clarity, but it's harmless if you'd rather leave it in.
|
sorry, been afk, had some life stuff happen, will review and try to get these answered promptly |
Drop the env-var indirection on the build.func source URL; it was a dev convenience for testing against a fork and adds no user-facing value.
3 participants
✍️ Description
NOTE: I have weighed using
node:ltsas the image tag, however that would make the CI runner not deploy in the same manner every time, undermining a core principle in the DevOps world: Reproducible Builds.🔗 Related PR / Issue
Still requires testing, will do so following the test plan outlined below in Additional Information✅ Prerequisites (X in brackets)
🛠️ Type of Change (X in brackets)
README,AppName.md,CONTRIBUTING.md, or other docs.🔍 Code & Security Review (X in brackets)
Code_Audit.md&CONTRIBUTING.mdguidelinesAppName.sh,AppName-install.sh,AppName.json)📋 Additional Information
Test plan
- Unattended install with mode="generated" and all var_* set: verify no prompts, container created with correct configTest 4 has been withheld at this time, pending question asked in issue Forgejo-Runner #1576 (comment)Once steps have been tested (and gif of output provided here) I will update and remove DRAFT, indicating ready for merge.
📦 Application Requirements (for new scripts)
brevity removal
🌐 Source
brevity removal