From bd4686e74d966b6d5e2fb14db2c51ff636ab8169 Mon Sep 17 00:00:00 2001 From: eliran-mic Date: Thu, 30 Apr 2026 13:42:53 +0300 Subject: [PATCH 1/4] fix(skyhook): align resolver with canonical schema; stop overriding workflow inputs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three bugs in the skyhook config resolution path were causing every caller's `inputs.context` to be silently clobbered, regardless of what they actually configured in `.skyhook/skyhook.yaml`: 1. **Wrong field name.** The action read `buildTool.docker.contextPath`, but the canonical schema in `koala-backend/internal/conf/skyhook.go::SkyhookDockerBuild` defines the field as `buildContext`. Customers following the schema saw their context overrides ignored entirely. 2. **No root-level fallback.** The yq lookups only scanned `.services[]`, with no fallback to top-level `buildTool.docker.*`. Monorepos that defined a single repo-wide build context at the root never saw it applied. 3. **Unconditional `resolved_context=code`.** Every error / no-config path emitted `resolved_context=code`, and the consumer expression put `resolved_context` first in the `||` chain. Net effect: whenever `service_name` was set, the action's resolver would always override `inputs.context` with `code`, even when the calling workflow had carefully resolved the right value (e.g. PR koala-backend#1319's new bash resolver in `build_image.yml`). This change: - Reads canonical `buildContext` first, falls back to legacy `contextPath` for back-compat, and emits a one-time deprecation warning when the legacy name is what got picked up. - Adds the missing root-level fallback (per-service > root > empty), mirroring `SkyhookConfig.ResolveBuildContext`. - Normalizes `.` and `./` to "no override" — same behaviour as the canonical Go resolver and the workflow-side resolver in `build_image.yml`. Keeps all three layers in agreement. - Stops emitting `resolved_context` when nothing in skyhook.yaml applies, so the consumer's `||` correctly falls through to `inputs.context` / `inputs.dockerfile`. - Strips a leading `./` from extracted values so we emit `code/src` instead of `code/./src`. The resolver bash is extracted to `scripts/resolve_skyhook_config.sh` so it can be unit-tested independently of GitHub Actions. A new CI job (`test-skyhook-resolver-unit`) runs eight cases covering canonical vs. legacy field names, root-vs-service precedence, the `./` normalization, the no-override-anywhere case, and unknown / empty service names. Uses `yq v4`'s `strenv()` for env-var injection — the jq-style `--arg` flag is not supported on yq v4 and silently produces wrong queries. The shipped `.skyhook/skyhook.yaml` test fixture is extended to exercise the canonical field, the legacy field, and the root-level fallback simultaneously. The pre-existing docker-based skyhook tests in `.github/workflows/test.yml` have been failing on `main` for an unrelated reason (`working-directory: code` doesn't exist when the test workflow checks out at the repo root, not under `code/`); this PR does not touch that. Made-with: Cursor --- .github/workflows/test.yml | 14 ++ .skyhook/skyhook.yaml | 34 ++++- action.yml | 80 ++--------- scripts/resolve_skyhook_config.sh | 126 +++++++++++++++++ test/unit/fixtures/canonical-only.yaml | 7 + test/unit/fixtures/dotslash-context.yaml | 10 ++ test/unit/fixtures/legacy-contextpath.yaml | 6 + test/unit/fixtures/no-buildtool-anywhere.yaml | 3 + test/unit/fixtures/root-and-services.yaml | 15 ++ test/unit/test_resolve_skyhook_config.sh | 133 ++++++++++++++++++ 10 files changed, 355 insertions(+), 73 deletions(-) create mode 100755 scripts/resolve_skyhook_config.sh create mode 100644 test/unit/fixtures/canonical-only.yaml create mode 100644 test/unit/fixtures/dotslash-context.yaml create mode 100644 test/unit/fixtures/legacy-contextpath.yaml create mode 100644 test/unit/fixtures/no-buildtool-anywhere.yaml create mode 100644 test/unit/fixtures/root-and-services.yaml create mode 100755 test/unit/test_resolve_skyhook_config.sh diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 1e80f11..a88f579 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -8,6 +8,20 @@ on: workflow_dispatch: jobs: + # Unit tests for the skyhook config resolver. These run in seconds and don't + # need Docker, so they're the first line of defence against regressions in + # field-name handling, root-vs-service precedence, and the override semantics + # of `resolved_context` / `resolved_dockerfile`. ubuntu-latest ships yq v4. + test-skyhook-resolver-unit: + runs-on: ubuntu-latest + name: Unit tests — skyhook config resolver + steps: + - name: Checkout + uses: actions/checkout@v4 + + - name: Run resolver unit tests + run: bash test/unit/test_resolve_skyhook_config.sh + test-basic-build: runs-on: ubuntu-latest name: Test basic Docker build diff --git a/.skyhook/skyhook.yaml b/.skyhook/skyhook.yaml index 4eaac1d..4c04e75 100644 --- a/.skyhook/skyhook.yaml +++ b/.skyhook/skyhook.yaml @@ -1,12 +1,35 @@ -# Test configuration for skyhook config resolution tests -# All paths are relative to the repository root +# Test configuration for skyhook config resolution tests. +# All paths are repo-root-relative (canonical schema, see +# koala-backend/internal/conf/skyhook.go::SkyhookDockerBuild). +# +# Exercises every branch of the resolver: +# - canonical per-service `buildContext` (web) +# - legacy per-service `contextPath` (api, worker) — back-compat path +# - root-level `buildContext` inherited by service (inherits-root) +# - service with no buildTool anywhere (simple) — defers to inputs + +buildTool: + docker: + # Root-level default. Services that omit buildTool entirely should + # inherit this. Per-service overrides take precedence. + buildContext: test/services/shared + services: + - name: web + path: test/services/web + buildTool: + docker: + # Canonical field name. Should win over the root buildContext above. + buildContext: test/services/web + - name: api path: test/services/api deploymentRepo: my-org/deployment deploymentRepoPath: api buildTool: docker: + # Legacy field name. Kept here so the resolver's deprecation / + # back-compat path stays exercised in tests. contextPath: test/services/api - name: worker @@ -16,6 +39,11 @@ services: contextPath: test/services/worker dockerfilePath: test/services/worker/docker/Dockerfile + - name: inherits-root + path: test/services/inherits-root + # No per-service buildTool — should fall back to the root buildContext. + - name: simple path: test/services/simple - # No buildTool defined - should use path as context fallback + # No per-service buildTool — same as inherits-root above; documents that + # the legacy fixture name still works without changes after the rewrite. diff --git a/action.yml b/action.yml index c7c75a1..7a7a4bf 100644 --- a/action.yml +++ b/action.yml @@ -198,76 +198,16 @@ runs: id: skyhook_config shell: bash working-directory: code - run: | - set -euo pipefail - - # Use the input if it exists; - if [[ -n "${{ inputs.service_name }}" ]]; then - SERVICE_NAME="${{ inputs.service_name }}" - fi - - # If no service_name provided, skip config resolution - if [[ -z "$SERVICE_NAME" ]]; then - echo "No service_name provided, skipping config resolution" - echo "resolved_context=code" >> "$GITHUB_OUTPUT" - exit 0 - fi - - # Find the config file - CONFIG_FILE="" - if [[ -f ".skyhook/skyhook.yaml" ]]; then - CONFIG_FILE=".skyhook/skyhook.yaml" - fi - - if [[ -z "$CONFIG_FILE" ]]; then - echo "::warning::service_name '$SERVICE_NAME' provided but .skyhook/skyhook.yaml not found. Falling back to input parameters." - echo "resolved_context=code" >> "$GITHUB_OUTPUT" - exit 0 - fi - - echo "Found config file: $CONFIG_FILE" - - # Check if service exists - SERVICE_EXISTS=$(yq ".services[] | select(.name == \"$SERVICE_NAME\") | .name" "$CONFIG_FILE") - if [[ -z "$SERVICE_EXISTS" ]]; then - echo "::warning::Service '$SERVICE_NAME' not found in $CONFIG_FILE. Falling back to input parameters." - echo "resolved_context=code" >> "$GITHUB_OUTPUT" - exit 0 - fi - - echo "Found service '$SERVICE_NAME' in config" - - # Extract build config using yq - # Context: only use buildTool.docker.contextPath if explicitly set, otherwise empty (defaults to repo root) - CONTEXT_PATH=$(yq ".services[] | select(.name == \"$SERVICE_NAME\") | .buildTool.docker.contextPath // \"\"" "$CONFIG_FILE") - DOCKERFILE_PATH=$(yq ".services[] | select(.name == \"$SERVICE_NAME\") | .buildTool.docker.dockerfilePath // \"\"" "$CONFIG_FILE") - - # Normalize outputs to repo root (this step runs in working-directory: code) - REPO_PREFIX="code" - - # Context: if contextPath is set use code/{contextPath}, else use code - if [[ -n "$CONTEXT_PATH" ]]; then - RESOLVED_CONTEXT="$REPO_PREFIX/$CONTEXT_PATH" - - # If dockerfile is not set, default to {context}/Dockerfile - if [[ -z "$DOCKERFILE_PATH" ]]; then - DOCKERFILE_PATH="${CONTEXT_PATH}/Dockerfile" - echo "Defaulting dockerfile to: $REPO_PREFIX/$DOCKERFILE_PATH" - fi - else - RESOLVED_CONTEXT="$REPO_PREFIX" - fi - echo "Using context: $RESOLVED_CONTEXT" - echo "resolved_context=$RESOLVED_CONTEXT" >> "$GITHUB_OUTPUT" - - if [[ -n "$DOCKERFILE_PATH" ]]; then - FULL_DOCKERFILE_PATH="$REPO_PREFIX/$DOCKERFILE_PATH" - echo "Using dockerfile from config: $FULL_DOCKERFILE_PATH" - echo "resolved_dockerfile=$FULL_DOCKERFILE_PATH" >> "$GITHUB_OUTPUT" - fi - - echo "config_file=$CONFIG_FILE" >> "$GITHUB_OUTPUT" - echo "service_name=$SERVICE_NAME" >> "$GITHUB_OUTPUT" + env: + SERVICE_NAME: ${{ inputs.service_name }} + # Path prefix the consumer expects the resolved values to live under. + # This step runs inside `code/` (the calling workflow's checkout dir), + # so any path read from skyhook.yaml is repo-root-relative and gets + # `code/` prepended before being emitted as an output. + REPO_PREFIX: code + # Resolver lives in scripts/ so it can be unit-tested independently of + # GitHub Actions (see test/unit/test_resolve_skyhook_config.sh). + run: bash "$GITHUB_ACTION_PATH/scripts/resolve_skyhook_config.sh" - name: Validate inputs shell: bash diff --git a/scripts/resolve_skyhook_config.sh b/scripts/resolve_skyhook_config.sh new file mode 100755 index 0000000..95bd0b5 --- /dev/null +++ b/scripts/resolve_skyhook_config.sh @@ -0,0 +1,126 @@ +#!/usr/bin/env bash +# +# Resolve Docker build context + Dockerfile from `.skyhook/skyhook.yaml`. +# +# Resolution order (mirrors koala-backend's SkyhookConfig.ResolveBuildContext): +# per-service buildTool.docker. -> +# root buildTool.docker. -> +# (consumer's `||` falls through to inputs.context / inputs.dockerfile) +# +# Field names: prefer canonical `buildContext`; fall back to legacy +# `contextPath` for back-compat (with a deprecation warning). `dockerfilePath` +# has no historical alias. +# +# We never preset `resolved_context=code` — that previously clobbered any +# value the calling workflow had already resolved for `inputs.context`. +# When skyhook.yaml provides nothing, we emit nothing and let inputs win. +# +# Inputs (env vars): +# SERVICE_NAME Service name to look up. If empty, the script is a no-op. +# REPO_PREFIX Path the consumer expects values to live under (default: "code"). +# SKYHOOK_FILE Optional override for the config file path +# (default: "$PWD/.skyhook/skyhook.yaml"). +# GITHUB_OUTPUT File to append `key=value` outputs to (GHA-compatible). +# If unset, outputs go to stdout instead. +# +# Requires `yq v4` (Mike Farah's Go yq) on PATH. `strenv()` is yq v4's env-var +# injection primitive — the jq-style `--arg` flag is NOT supported on yq v4 +# and silently produces wrong queries. + +set -euo pipefail + +: "${SERVICE_NAME:=}" +: "${REPO_PREFIX:=code}" +: "${SKYHOOK_FILE:=.skyhook/skyhook.yaml}" + +# Direct emitted output to GHA's $GITHUB_OUTPUT when present, else stdout. +emit() { + if [[ -n "${GITHUB_OUTPUT:-}" ]]; then + printf '%s\n' "$1" >> "$GITHUB_OUTPUT" + else + printf '%s\n' "$1" + fi +} + +log() { printf '%s\n' "$*" >&2; } + +# yq_get — returns "" for missing/null and normalizes "."/"./". +yq_get() { + local q=$1 file=$2 out + out=$(yq "$q" "$file" 2>/dev/null || true) + # yq emits the literal string "null" when a path resolves to a missing key + # and `// ""` did not absorb it (e.g. when a parent path is itself absent). + [[ "$out" == "null" ]] && out="" + # Treat "." and "./" as "no override": both `koala-backend` (canonical + # resolver) and the workflow-side resolver in `build_image.yml` collapse + # them. Keeping the same semantic here so all three layers agree. + case "$out" in .|./) out="" ;; esac + # Strip a leading `./` so we emit `code/src` instead of `code/./src`. + out=${out#./} + printf '%s' "$out" +} + +if [[ -z "$SERVICE_NAME" ]]; then + log "No service_name provided; skipping skyhook config resolution" + exit 0 +fi + +if [[ ! -f "$SKYHOOK_FILE" ]]; then + log "::warning::service_name '$SERVICE_NAME' was provided but $SKYHOOK_FILE was not found. Using inputs.context / inputs.dockerfile as-is." + exit 0 +fi + +log "Found config file: $SKYHOOK_FILE" + +# Service must exist in the config; otherwise we don't second-guess the inputs. +SERVICE_EXISTS=$(yq '(.services // []) | map(select(.name == strenv(SERVICE_NAME))) | .[0].name // ""' "$SKYHOOK_FILE" 2>/dev/null || true) +[[ "$SERVICE_EXISTS" == "null" ]] && SERVICE_EXISTS="" +if [[ -z "$SERVICE_EXISTS" ]]; then + log "::warning::Service '$SERVICE_NAME' not found in $SKYHOOK_FILE. Using inputs.context / inputs.dockerfile as-is." + exit 0 +fi +log "Found service '$SERVICE_NAME' in config" + +SVC_CTX=$(yq_get '(.services // []) | map(select(.name == strenv(SERVICE_NAME))) | (.[0].buildTool.docker.buildContext // "")' "$SKYHOOK_FILE") +SVC_CTX_LEGACY=$(yq_get '(.services // []) | map(select(.name == strenv(SERVICE_NAME))) | (.[0].buildTool.docker.contextPath // "")' "$SKYHOOK_FILE") +SVC_DFP=$(yq_get '(.services // []) | map(select(.name == strenv(SERVICE_NAME))) | (.[0].buildTool.docker.dockerfilePath // "")' "$SKYHOOK_FILE") + +ROOT_CTX=$(yq_get '.buildTool.docker.buildContext // ""' "$SKYHOOK_FILE") +ROOT_CTX_LEGACY=$(yq_get '.buildTool.docker.contextPath // ""' "$SKYHOOK_FILE") +ROOT_DFP=$(yq_get '.buildTool.docker.dockerfilePath // ""' "$SKYHOOK_FILE") + +# One deprecation warning if anyone is still on `contextPath` and would +# actually be picked up after the canonical `buildContext` lookup misses. +if { [[ -z "$SVC_CTX" && -n "$SVC_CTX_LEGACY" ]] || [[ -z "$SVC_CTX" && -z "$ROOT_CTX" && -n "$ROOT_CTX_LEGACY" ]]; }; then + log "::warning::buildTool.docker.contextPath is deprecated; rename to buildContext (see https://github.com/skyhook-io/docker-build-push-action#skyhook-config)." +fi + +# Apply resolution order: service > root > empty. +CTX="${SVC_CTX:-${SVC_CTX_LEGACY:-${ROOT_CTX:-$ROOT_CTX_LEGACY}}}" +DFP="${SVC_DFP:-$ROOT_DFP}" + +if [[ -n "$CTX" ]]; then + RESOLVED_CONTEXT="$REPO_PREFIX/$CTX" + # When context is overridden but no Dockerfile is specified anywhere, + # default the Dockerfile to /Dockerfile (same as Docker's + # built-in default, but anchored to the resolved context). + if [[ -z "$DFP" ]]; then + DFP="$CTX/Dockerfile" + log "Defaulting dockerfile to: $REPO_PREFIX/$DFP" + fi + log "Using context: $RESOLVED_CONTEXT" + emit "resolved_context=$RESOLVED_CONTEXT" +else + log "No buildContext override in skyhook.yaml; deferring to inputs.context" +fi + +if [[ -n "$DFP" ]]; then + RESOLVED_DOCKERFILE="$REPO_PREFIX/$DFP" + log "Using dockerfile: $RESOLVED_DOCKERFILE" + emit "resolved_dockerfile=$RESOLVED_DOCKERFILE" +else + log "No dockerfilePath override in skyhook.yaml; deferring to inputs.dockerfile" +fi + +emit "config_file=$SKYHOOK_FILE" +emit "service_name=$SERVICE_NAME" diff --git a/test/unit/fixtures/canonical-only.yaml b/test/unit/fixtures/canonical-only.yaml new file mode 100644 index 0000000..004b5d5 --- /dev/null +++ b/test/unit/fixtures/canonical-only.yaml @@ -0,0 +1,7 @@ +services: + - name: web + path: apps/web + buildTool: + docker: + buildContext: apps/web/src + dockerfilePath: apps/web/docker/Dockerfile diff --git a/test/unit/fixtures/dotslash-context.yaml b/test/unit/fixtures/dotslash-context.yaml new file mode 100644 index 0000000..ed0cd4f --- /dev/null +++ b/test/unit/fixtures/dotslash-context.yaml @@ -0,0 +1,10 @@ +services: + - name: monorepo-root + path: apps/foo + buildTool: + docker: + # `./` and `.` are normalized to "no override" by the resolver — same as + # koala-backend's SkyhookConfig.ResolveBuildContext and the workflow-side + # resolver in build_image.yml. Documented limitation. + buildContext: ./ + dockerfilePath: apps/foo/Dockerfile diff --git a/test/unit/fixtures/legacy-contextpath.yaml b/test/unit/fixtures/legacy-contextpath.yaml new file mode 100644 index 0000000..85f1d26 --- /dev/null +++ b/test/unit/fixtures/legacy-contextpath.yaml @@ -0,0 +1,6 @@ +services: + - name: legacy + path: apps/legacy + buildTool: + docker: + contextPath: apps/legacy diff --git a/test/unit/fixtures/no-buildtool-anywhere.yaml b/test/unit/fixtures/no-buildtool-anywhere.yaml new file mode 100644 index 0000000..256b7de --- /dev/null +++ b/test/unit/fixtures/no-buildtool-anywhere.yaml @@ -0,0 +1,3 @@ +services: + - name: bare + path: apps/bare diff --git a/test/unit/fixtures/root-and-services.yaml b/test/unit/fixtures/root-and-services.yaml new file mode 100644 index 0000000..72bb74a --- /dev/null +++ b/test/unit/fixtures/root-and-services.yaml @@ -0,0 +1,15 @@ +buildTool: + docker: + buildContext: shared + dockerfilePath: shared/Dockerfile + +services: + - name: web + path: apps/web + buildTool: + docker: + buildContext: apps/web/src + + - name: api + path: apps/api + # No per-service buildTool — should inherit the root values. diff --git a/test/unit/test_resolve_skyhook_config.sh b/test/unit/test_resolve_skyhook_config.sh new file mode 100755 index 0000000..aadb50c --- /dev/null +++ b/test/unit/test_resolve_skyhook_config.sh @@ -0,0 +1,133 @@ +#!/usr/bin/env bash +# +# Unit tests for scripts/resolve_skyhook_config.sh. +# +# Each case sets env vars for the resolver, runs it against a fixture YAML in +# fixtures/, and asserts the contents written to a fake $GITHUB_OUTPUT. +# +# Run from anywhere: +# bash test/unit/test_resolve_skyhook_config.sh +# +# Requires `yq v4` on PATH (same requirement as the action itself). + +set -euo pipefail + +REPO_ROOT="$(cd "$(dirname "$0")/../.." && pwd)" +RESOLVER="$REPO_ROOT/scripts/resolve_skyhook_config.sh" +FIXTURES="$REPO_ROOT/test/unit/fixtures" +TMPROOT="$(mktemp -d)" +trap 'rm -rf "$TMPROOT"' EXIT + +[[ -x "$RESOLVER" ]] || chmod +x "$RESOLVER" + +if ! command -v yq >/dev/null; then + echo "yq is required" >&2 + exit 2 +fi +if ! yq --version 2>&1 | grep -q 'version v4'; then + echo "yq v4 is required, found: $(yq --version 2>&1)" >&2 + exit 2 +fi + +pass=0; fail=0 + +# run_case +# +# expected_outputs_grep_regex is a `;`-separated list of grep-E patterns that +# must each match a line in $GITHUB_OUTPUT. Prefix a pattern with `!` to assert +# it MUST NOT match. +run_case() { + local name=$1 fixture=$2 svc=$3 expects=$4 + local case_dir out_file + case_dir="$TMPROOT/$name" + mkdir -p "$case_dir" + out_file="$case_dir/github_output" + : > "$out_file" + + SERVICE_NAME="$svc" \ + REPO_PREFIX="code" \ + SKYHOOK_FILE="$FIXTURES/$fixture" \ + GITHUB_OUTPUT="$out_file" \ + bash "$RESOLVER" >"$case_dir/stdout" 2>"$case_dir/stderr" + + local ok=1 + local IFS=';' + for pattern in $expects; do + pattern="${pattern# }"; pattern="${pattern% }" + [[ -z "$pattern" ]] && continue + if [[ "${pattern:0:1}" == "!" ]]; then + local p="${pattern:1}" + if grep -Eq "$p" "$out_file"; then + printf ' FAIL %s: unexpected output line matching /%s/\n' "$name" "$p" + ok=0 + fi + else + if ! grep -Eq "$pattern" "$out_file"; then + printf ' FAIL %s: missing output line matching /%s/\n' "$name" "$pattern" + ok=0 + fi + fi + done + + if [[ "$ok" == "1" ]]; then + printf ' ok %s\n' "$name" + pass=$((pass + 1)) + else + printf ' ---- $GITHUB_OUTPUT ----\n' + sed 's/^/ /' "$out_file" + printf ' ---- stderr ----\n' + sed 's/^/ /' "$case_dir/stderr" + fail=$((fail + 1)) + fi +} + +echo "running unit tests against $RESOLVER" + +# 1. Canonical buildContext + dockerfilePath are emitted with code/ prefix. +run_case "canonical-fields-emitted" \ + "canonical-only.yaml" "web" \ + "^resolved_context=code/apps/web/src$ ; ^resolved_dockerfile=code/apps/web/docker/Dockerfile$" + +# 2. Legacy contextPath is honored (back-compat path). +run_case "legacy-contextpath-honored" \ + "legacy-contextpath.yaml" "legacy" \ + "^resolved_context=code/apps/legacy$ ; ^resolved_dockerfile=code/apps/legacy/Dockerfile$" + +# 3. Per-service value overrides root. +run_case "service-overrides-root" \ + "root-and-services.yaml" "web" \ + "^resolved_context=code/apps/web/src$ ; !^resolved_context=code/shared$" + +# 4. Service with no per-service buildTool inherits root buildContext / dockerfilePath. +run_case "service-inherits-root" \ + "root-and-services.yaml" "api" \ + "^resolved_context=code/shared$ ; ^resolved_dockerfile=code/shared/Dockerfile$" + +# 5. No buildTool anywhere → no resolved_* outputs (consumer's || falls through to inputs). +run_case "no-override-anywhere" \ + "no-buildtool-anywhere.yaml" "bare" \ + "!^resolved_context= ; !^resolved_dockerfile=" + +# 6. `./` is normalized to "no override" — only dockerfilePath is emitted. +run_case "dotslash-context-is-normalized" \ + "dotslash-context.yaml" "monorepo-root" \ + "!^resolved_context= ; ^resolved_dockerfile=code/apps/foo/Dockerfile$" + +# 7. Empty service_name → no-op (no outputs at all). +run_case "no-service-name-is-noop" \ + "canonical-only.yaml" "" \ + "!^resolved_context= ; !^resolved_dockerfile= ; !^config_file= ; !^service_name=" + +# 8. Service not found → no-op (resolver doesn't second-guess inputs). +run_case "unknown-service-is-noop" \ + "canonical-only.yaml" "does-not-exist" \ + "!^resolved_context= ; !^resolved_dockerfile= ; !^config_file= ; !^service_name=" + +echo +if [[ "$fail" -eq 0 ]]; then + echo "ALL $pass UNIT TESTS PASSED" + exit 0 +else + echo "$fail/$((pass+fail)) UNIT TESTS FAILED" + exit 1 +fi From 3085664c40a6ce6c371139ebbab7815b114a5005 Mon Sep 17 00:00:00 2001 From: eliran-mic Date: Thu, 30 Apr 2026 14:00:09 +0300 Subject: [PATCH 2/4] Add SERVICE_DIR + code fallbacks; always emit resolved context/dockerfile MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Aligns the action's resolution chain with koala-backend PR #1319's workflow-side resolver so both layers compute the same value: step │ context │ dockerfile ─────┼──────────────────────────┼───────────────────────────────── 1 │ per-service buildContext │ per-service dockerfilePath 2 │ root buildContext │ root dockerfilePath 3 │ code │ code//Dockerfile 4 │ code │ code/Dockerfile (no SERVICE_DIR) Notable choices: - Context has no SERVICE_DIR step. The build context defaults to the entire checkout when nothing in YAML overrides it; .dockerignore / Dockerfile COPY paths govern what's actually included. Avoids the surprise of narrowing context to a sub-directory the user didn't ask for. - Dockerfile chains independently of context. When only buildContext is overridden, the Dockerfile still falls through to code//Dockerfile (or code/Dockerfile) — it does NOT auto-default to /Dockerfile. - The resolver now ALWAYS emits resolved_context / resolved_dockerfile whenever service_name is set. Manual mode (no service_name) is unchanged: noop, consumer's `||` falls through to inputs.context / inputs.dockerfile. Bug 3 ("action overrides workflow input") becomes moot because both resolvers compute the same value. A new optional `service_dir` action input feeds the SERVICE_DIR fallback for callers that want it (e.g. koala-backend's generated `build_image.yml`). Standalone callers can omit it; the dockerfile then defaults to code/Dockerfile. Unit tests: - Replaced the "no-override-anywhere → no outputs" case with two new cases that pin down the SERVICE_DIR vs no-SERVICE_DIR fallback. - Updated the legacy-contextPath case to assert the dockerfile falls through to code/Dockerfile (proving context and dockerfile are resolved independently). - Updated the "unknown service" case to assert the SERVICE_DIR-based dockerfile fallback still applies. 9/9 cases green locally. Made-with: Cursor --- action.yml | 8 ++ scripts/resolve_skyhook_config.sh | 121 +++++++++++++---------- test/unit/test_resolve_skyhook_config.sh | 67 ++++++++----- 3 files changed, 120 insertions(+), 76 deletions(-) diff --git a/action.yml b/action.yml index 7a7a4bf..c3ca9dc 100644 --- a/action.yml +++ b/action.yml @@ -11,6 +11,13 @@ inputs: service_name: description: 'Service name to build, as defined in .skyhook/skyhook.yaml (services[].name)' required: false + service_dir: + description: | + Service directory inside the repo (e.g. "apps/web"). Used as the SERVICE_DIR + fallback when no dockerfilePath override is set in skyhook.yaml: the action + then defaults the Dockerfile to `code//Dockerfile`. Has no + effect on the build context (context falls back to `code`, not `code/`). + required: false # Manual mode — explicit list of image:tag pairs (newline-delimited). Mix registries as needed. tags: @@ -200,6 +207,7 @@ runs: working-directory: code env: SERVICE_NAME: ${{ inputs.service_name }} + SERVICE_DIR: ${{ inputs.service_dir }} # Path prefix the consumer expects the resolved values to live under. # This step runs inside `code/` (the calling workflow's checkout dir), # so any path read from skyhook.yaml is repo-root-relative and gets diff --git a/scripts/resolve_skyhook_config.sh b/scripts/resolve_skyhook_config.sh index 95bd0b5..7272a02 100755 --- a/scripts/resolve_skyhook_config.sh +++ b/scripts/resolve_skyhook_config.sh @@ -2,21 +2,29 @@ # # Resolve Docker build context + Dockerfile from `.skyhook/skyhook.yaml`. # -# Resolution order (mirrors koala-backend's SkyhookConfig.ResolveBuildContext): -# per-service buildTool.docker. -> -# root buildTool.docker. -> -# (consumer's `||` falls through to inputs.context / inputs.dockerfile) +# Resolution chain (mirrors koala-backend/.../build_image.yml — PR #1319): +# +# step │ context │ dockerfile +# ─────┼───────────────────────────┼────────────────────────────────── +# 1 │ per-service buildContext │ per-service dockerfilePath +# 2 │ root buildContext │ root dockerfilePath +# 3 │ code │ code//Dockerfile +# 4 │ code │ code/Dockerfile (no SERVICE_DIR) +# +# YAML values are repo-root-relative ("absolute from repo root"); the script +# only ever prepends `$REPO_PREFIX` (the calling workflow's checkout dir). # # Field names: prefer canonical `buildContext`; fall back to legacy # `contextPath` for back-compat (with a deprecation warning). `dockerfilePath` # has no historical alias. # -# We never preset `resolved_context=code` — that previously clobbered any -# value the calling workflow had already resolved for `inputs.context`. -# When skyhook.yaml provides nothing, we emit nothing and let inputs win. -# # Inputs (env vars): -# SERVICE_NAME Service name to look up. If empty, the script is a no-op. +# SERVICE_NAME Service name to look up. If empty, the script is a no-op +# (manual mode — caller's `inputs.context` / `inputs.dockerfile` +# are used as-is). +# SERVICE_DIR Service directory inside the repo (typically the same as +# skyhook.yaml's `services[].path`). Optional; only affects +# the dockerfile fallback (step 3 above). # REPO_PREFIX Path the consumer expects values to live under (default: "code"). # SKYHOOK_FILE Optional override for the config file path # (default: "$PWD/.skyhook/skyhook.yaml"). @@ -30,10 +38,10 @@ set -euo pipefail : "${SERVICE_NAME:=}" +: "${SERVICE_DIR:=}" : "${REPO_PREFIX:=code}" : "${SKYHOOK_FILE:=.skyhook/skyhook.yaml}" -# Direct emitted output to GHA's $GITHUB_OUTPUT when present, else stdout. emit() { if [[ -n "${GITHUB_OUTPUT:-}" ]]; then printf '%s\n' "$1" >> "$GITHUB_OUTPUT" @@ -60,67 +68,78 @@ yq_get() { printf '%s' "$out" } +# Manual mode: nothing to resolve, leave inputs alone. if [[ -z "$SERVICE_NAME" ]]; then log "No service_name provided; skipping skyhook config resolution" exit 0 fi -if [[ ! -f "$SKYHOOK_FILE" ]]; then - log "::warning::service_name '$SERVICE_NAME' was provided but $SKYHOOK_FILE was not found. Using inputs.context / inputs.dockerfile as-is." - exit 0 -fi - -log "Found config file: $SKYHOOK_FILE" - -# Service must exist in the config; otherwise we don't second-guess the inputs. -SERVICE_EXISTS=$(yq '(.services // []) | map(select(.name == strenv(SERVICE_NAME))) | .[0].name // ""' "$SKYHOOK_FILE" 2>/dev/null || true) -[[ "$SERVICE_EXISTS" == "null" ]] && SERVICE_EXISTS="" -if [[ -z "$SERVICE_EXISTS" ]]; then - log "::warning::Service '$SERVICE_NAME' not found in $SKYHOOK_FILE. Using inputs.context / inputs.dockerfile as-is." - exit 0 +# Pull root-level overrides up front: they apply even when no per-service +# config (or no service entry) exists. +ROOT_CTX="" +ROOT_CTX_LEGACY="" +ROOT_DFP="" +SVC_CTX="" +SVC_CTX_LEGACY="" +SVC_DFP="" +CONFIG_PRESENT=0 + +if [[ -f "$SKYHOOK_FILE" ]]; then + CONFIG_PRESENT=1 + log "Found config file: $SKYHOOK_FILE" + + ROOT_CTX=$(yq_get '.buildTool.docker.buildContext // ""' "$SKYHOOK_FILE") + ROOT_CTX_LEGACY=$(yq_get '.buildTool.docker.contextPath // ""' "$SKYHOOK_FILE") + ROOT_DFP=$(yq_get '.buildTool.docker.dockerfilePath // ""' "$SKYHOOK_FILE") + + SERVICE_EXISTS=$(yq '(.services // []) | map(select(.name == strenv(SERVICE_NAME))) | .[0].name // ""' "$SKYHOOK_FILE" 2>/dev/null || true) + [[ "$SERVICE_EXISTS" == "null" ]] && SERVICE_EXISTS="" + if [[ -n "$SERVICE_EXISTS" ]]; then + log "Found service '$SERVICE_NAME' in config" + SVC_CTX=$(yq_get '(.services // []) | map(select(.name == strenv(SERVICE_NAME))) | (.[0].buildTool.docker.buildContext // "")' "$SKYHOOK_FILE") + SVC_CTX_LEGACY=$(yq_get '(.services // []) | map(select(.name == strenv(SERVICE_NAME))) | (.[0].buildTool.docker.contextPath // "")' "$SKYHOOK_FILE") + SVC_DFP=$(yq_get '(.services // []) | map(select(.name == strenv(SERVICE_NAME))) | (.[0].buildTool.docker.dockerfilePath // "")' "$SKYHOOK_FILE") + else + log "::warning::Service '$SERVICE_NAME' not found in $SKYHOOK_FILE; only root-level overrides (if any) will apply." + fi +else + log "::warning::service_name '$SERVICE_NAME' was provided but $SKYHOOK_FILE was not found; using SERVICE_DIR / code defaults." fi -log "Found service '$SERVICE_NAME' in config" - -SVC_CTX=$(yq_get '(.services // []) | map(select(.name == strenv(SERVICE_NAME))) | (.[0].buildTool.docker.buildContext // "")' "$SKYHOOK_FILE") -SVC_CTX_LEGACY=$(yq_get '(.services // []) | map(select(.name == strenv(SERVICE_NAME))) | (.[0].buildTool.docker.contextPath // "")' "$SKYHOOK_FILE") -SVC_DFP=$(yq_get '(.services // []) | map(select(.name == strenv(SERVICE_NAME))) | (.[0].buildTool.docker.dockerfilePath // "")' "$SKYHOOK_FILE") -ROOT_CTX=$(yq_get '.buildTool.docker.buildContext // ""' "$SKYHOOK_FILE") -ROOT_CTX_LEGACY=$(yq_get '.buildTool.docker.contextPath // ""' "$SKYHOOK_FILE") -ROOT_DFP=$(yq_get '.buildTool.docker.dockerfilePath // ""' "$SKYHOOK_FILE") - -# One deprecation warning if anyone is still on `contextPath` and would -# actually be picked up after the canonical `buildContext` lookup misses. +# One-shot deprecation warning if anyone is still on `contextPath` AND the +# canonical name didn't already win at the same scope. if { [[ -z "$SVC_CTX" && -n "$SVC_CTX_LEGACY" ]] || [[ -z "$SVC_CTX" && -z "$ROOT_CTX" && -n "$ROOT_CTX_LEGACY" ]]; }; then log "::warning::buildTool.docker.contextPath is deprecated; rename to buildContext (see https://github.com/skyhook-io/docker-build-push-action#skyhook-config)." fi -# Apply resolution order: service > root > empty. +# ── Context chain ─────────────────────────────────────────────────────────── +# per-service > root > "code" (no SERVICE_DIR step on context — the build +# context defaults to the entire checkout, and `.dockerignore` / Dockerfile +# COPY paths govern what's actually included). CTX="${SVC_CTX:-${SVC_CTX_LEGACY:-${ROOT_CTX:-$ROOT_CTX_LEGACY}}}" -DFP="${SVC_DFP:-$ROOT_DFP}" - if [[ -n "$CTX" ]]; then RESOLVED_CONTEXT="$REPO_PREFIX/$CTX" - # When context is overridden but no Dockerfile is specified anywhere, - # default the Dockerfile to /Dockerfile (same as Docker's - # built-in default, but anchored to the resolved context). - if [[ -z "$DFP" ]]; then - DFP="$CTX/Dockerfile" - log "Defaulting dockerfile to: $REPO_PREFIX/$DFP" - fi - log "Using context: $RESOLVED_CONTEXT" - emit "resolved_context=$RESOLVED_CONTEXT" else - log "No buildContext override in skyhook.yaml; deferring to inputs.context" + RESOLVED_CONTEXT="$REPO_PREFIX" fi +log "Using context: $RESOLVED_CONTEXT" +emit "resolved_context=$RESOLVED_CONTEXT" +# ── Dockerfile chain ──────────────────────────────────────────────────────── +# per-service > root > /Dockerfile > Dockerfile. +DFP="${SVC_DFP:-$ROOT_DFP}" if [[ -n "$DFP" ]]; then RESOLVED_DOCKERFILE="$REPO_PREFIX/$DFP" - log "Using dockerfile: $RESOLVED_DOCKERFILE" - emit "resolved_dockerfile=$RESOLVED_DOCKERFILE" +elif [[ -n "$SERVICE_DIR" ]]; then + RESOLVED_DOCKERFILE="$REPO_PREFIX/$SERVICE_DIR/Dockerfile" else - log "No dockerfilePath override in skyhook.yaml; deferring to inputs.dockerfile" + RESOLVED_DOCKERFILE="$REPO_PREFIX/Dockerfile" fi +log "Using dockerfile: $RESOLVED_DOCKERFILE" +emit "resolved_dockerfile=$RESOLVED_DOCKERFILE" -emit "config_file=$SKYHOOK_FILE" +# Diagnostic outputs (consumed by the action's step summary). +if [[ "$CONFIG_PRESENT" == "1" ]]; then + emit "config_file=$SKYHOOK_FILE" +fi emit "service_name=$SERVICE_NAME" diff --git a/test/unit/test_resolve_skyhook_config.sh b/test/unit/test_resolve_skyhook_config.sh index aadb50c..62b5462 100755 --- a/test/unit/test_resolve_skyhook_config.sh +++ b/test/unit/test_resolve_skyhook_config.sh @@ -31,13 +31,13 @@ fi pass=0; fail=0 -# run_case +# run_case # -# expected_outputs_grep_regex is a `;`-separated list of grep-E patterns that -# must each match a line in $GITHUB_OUTPUT. Prefix a pattern with `!` to assert -# it MUST NOT match. +# expected_outputs is a `;`-separated list of grep-E patterns that must each +# match a line in $GITHUB_OUTPUT. Prefix a pattern with `!` to assert it MUST +# NOT match. run_case() { - local name=$1 fixture=$2 svc=$3 expects=$4 + local name=$1 fixture=$2 svc=$3 svc_dir=$4 expects=$5 local case_dir out_file case_dir="$TMPROOT/$name" mkdir -p "$case_dir" @@ -45,6 +45,7 @@ run_case() { : > "$out_file" SERVICE_NAME="$svc" \ + SERVICE_DIR="$svc_dir" \ REPO_PREFIX="code" \ SKYHOOK_FILE="$FIXTURES/$fixture" \ GITHUB_OUTPUT="$out_file" \ @@ -83,45 +84,61 @@ run_case() { echo "running unit tests against $RESOLVER" -# 1. Canonical buildContext + dockerfilePath are emitted with code/ prefix. +# ── Per-service overrides ────────────────────────────────────────────────── + +# 1. Canonical buildContext + dockerfilePath, both per-service. run_case "canonical-fields-emitted" \ - "canonical-only.yaml" "web" \ + "canonical-only.yaml" "web" "" \ "^resolved_context=code/apps/web/src$ ; ^resolved_dockerfile=code/apps/web/docker/Dockerfile$" -# 2. Legacy contextPath is honored (back-compat path). +# 2. Legacy contextPath honored (back-compat path). When dockerfilePath is +# unset, dockerfile falls all the way through to `code/Dockerfile` because +# no SERVICE_DIR was passed (NOT to `code//Dockerfile`; context +# and dockerfile chains are independent). run_case "legacy-contextpath-honored" \ - "legacy-contextpath.yaml" "legacy" \ - "^resolved_context=code/apps/legacy$ ; ^resolved_dockerfile=code/apps/legacy/Dockerfile$" + "legacy-contextpath.yaml" "legacy" "" \ + "^resolved_context=code/apps/legacy$ ; ^resolved_dockerfile=code/Dockerfile$" # 3. Per-service value overrides root. run_case "service-overrides-root" \ - "root-and-services.yaml" "web" \ + "root-and-services.yaml" "web" "" \ "^resolved_context=code/apps/web/src$ ; !^resolved_context=code/shared$" # 4. Service with no per-service buildTool inherits root buildContext / dockerfilePath. run_case "service-inherits-root" \ - "root-and-services.yaml" "api" \ + "root-and-services.yaml" "api" "" \ "^resolved_context=code/shared$ ; ^resolved_dockerfile=code/shared/Dockerfile$" -# 5. No buildTool anywhere → no resolved_* outputs (consumer's || falls through to inputs). -run_case "no-override-anywhere" \ - "no-buildtool-anywhere.yaml" "bare" \ - "!^resolved_context= ; !^resolved_dockerfile=" +# ── Fallback chain (no YAML override) ────────────────────────────────────── + +# 5. No buildTool anywhere, no SERVICE_DIR → context=code, dockerfile=code/Dockerfile. +run_case "no-override-no-service-dir" \ + "no-buildtool-anywhere.yaml" "bare" "" \ + "^resolved_context=code$ ; ^resolved_dockerfile=code/Dockerfile$" + +# 6. No buildTool anywhere, SERVICE_DIR set → context=code, dockerfile=code//Dockerfile. +run_case "no-override-with-service-dir" \ + "no-buildtool-anywhere.yaml" "bare" "apps/bare" \ + "^resolved_context=code$ ; ^resolved_dockerfile=code/apps/bare/Dockerfile$" -# 6. `./` is normalized to "no override" — only dockerfilePath is emitted. +# 7. `./` is normalized to "no override" — falls through to defaults. +# (dockerfilePath in the fixture is set, so dockerfile uses that.) run_case "dotslash-context-is-normalized" \ - "dotslash-context.yaml" "monorepo-root" \ - "!^resolved_context= ; ^resolved_dockerfile=code/apps/foo/Dockerfile$" + "dotslash-context.yaml" "monorepo-root" "apps/foo" \ + "^resolved_context=code$ ; ^resolved_dockerfile=code/apps/foo/Dockerfile$" -# 7. Empty service_name → no-op (no outputs at all). +# ── No-op edge cases ─────────────────────────────────────────────────────── + +# 8. Empty service_name → manual mode, no outputs at all. run_case "no-service-name-is-noop" \ - "canonical-only.yaml" "" \ + "canonical-only.yaml" "" "" \ "!^resolved_context= ; !^resolved_dockerfile= ; !^config_file= ; !^service_name=" -# 8. Service not found → no-op (resolver doesn't second-guess inputs). -run_case "unknown-service-is-noop" \ - "canonical-only.yaml" "does-not-exist" \ - "!^resolved_context= ; !^resolved_dockerfile= ; !^config_file= ; !^service_name=" +# 9. Service not found in YAML → root-level overrides (none in this fixture) +# + SERVICE_DIR-based dockerfile fallback. Diagnostic warning emitted. +run_case "unknown-service-uses-defaults" \ + "canonical-only.yaml" "does-not-exist" "apps/missing" \ + "^resolved_context=code$ ; ^resolved_dockerfile=code/apps/missing/Dockerfile$ ; ^service_name=does-not-exist$" echo if [[ "$fail" -eq 0 ]]; then From 95686e0b870ee61d7463633cc6eaf99a7903dd02 Mon Sep 17 00:00:00 2001 From: eliran-mic Date: Thu, 30 Apr 2026 14:02:59 +0300 Subject: [PATCH 3/4] =?UTF-8?q?Drop=20service=5Fdir=20input=20=E2=80=94=20?= =?UTF-8?q?read=20services[].path=20from=20skyhook.yaml=20directly?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit added a `service_dir` action input as the SERVICE_DIR fallback for the dockerfile chain. It was redundant: the action already reads the YAML, and the value `service_dir` was expected to carry is exactly `services[name=$SERVICE_NAME].path`. This commit removes the input and reads `services[].path` from the YAML directly. The chain semantics are unchanged: step │ context │ dockerfile ─────┼──────────────────────────┼──────────────────────────────── 1 │ per-service buildContext │ per-service dockerfilePath 2 │ root buildContext │ root dockerfilePath 3 │ code │ code//Dockerfile 4 │ code │ code/Dockerfile (no service.path) Net effect: - One fewer surface area on the action; service_name is the only skyhook-mode input now. - One fewer place where caller-passed value and YAML value can drift. - Test fixture extended with a second "no buildTool" service that also has no `path`, so step-4 (code/Dockerfile) has its own unit-test coverage. 9/9 unit tests still green. Made-with: Cursor --- action.yml | 8 --- scripts/resolve_skyhook_config.sh | 24 +++++---- test/unit/fixtures/no-buildtool-anywhere.yaml | 6 ++- test/unit/test_resolve_skyhook_config.sh | 52 +++++++++---------- 4 files changed, 45 insertions(+), 45 deletions(-) diff --git a/action.yml b/action.yml index c3ca9dc..7a7a4bf 100644 --- a/action.yml +++ b/action.yml @@ -11,13 +11,6 @@ inputs: service_name: description: 'Service name to build, as defined in .skyhook/skyhook.yaml (services[].name)' required: false - service_dir: - description: | - Service directory inside the repo (e.g. "apps/web"). Used as the SERVICE_DIR - fallback when no dockerfilePath override is set in skyhook.yaml: the action - then defaults the Dockerfile to `code//Dockerfile`. Has no - effect on the build context (context falls back to `code`, not `code/`). - required: false # Manual mode — explicit list of image:tag pairs (newline-delimited). Mix registries as needed. tags: @@ -207,7 +200,6 @@ runs: working-directory: code env: SERVICE_NAME: ${{ inputs.service_name }} - SERVICE_DIR: ${{ inputs.service_dir }} # Path prefix the consumer expects the resolved values to live under. # This step runs inside `code/` (the calling workflow's checkout dir), # so any path read from skyhook.yaml is repo-root-relative and gets diff --git a/scripts/resolve_skyhook_config.sh b/scripts/resolve_skyhook_config.sh index 7272a02..50c81ca 100755 --- a/scripts/resolve_skyhook_config.sh +++ b/scripts/resolve_skyhook_config.sh @@ -8,8 +8,8 @@ # ─────┼───────────────────────────┼────────────────────────────────── # 1 │ per-service buildContext │ per-service dockerfilePath # 2 │ root buildContext │ root dockerfilePath -# 3 │ code │ code//Dockerfile -# 4 │ code │ code/Dockerfile (no SERVICE_DIR) +# 3 │ code │ code//Dockerfile +# 4 │ code │ code/Dockerfile (no service.path) # # YAML values are repo-root-relative ("absolute from repo root"); the script # only ever prepends `$REPO_PREFIX` (the calling workflow's checkout dir). @@ -18,13 +18,15 @@ # `contextPath` for back-compat (with a deprecation warning). `dockerfilePath` # has no historical alias. # +# The "SERVICE_DIR" used in step 3 of the dockerfile chain is read from +# `services[name=$SERVICE_NAME].path` in skyhook.yaml — the action does not +# need it as a separate input. If the service has no `path`, step 3 is +# skipped and step 4 (`code/Dockerfile`) wins. +# # Inputs (env vars): # SERVICE_NAME Service name to look up. If empty, the script is a no-op # (manual mode — caller's `inputs.context` / `inputs.dockerfile` # are used as-is). -# SERVICE_DIR Service directory inside the repo (typically the same as -# skyhook.yaml's `services[].path`). Optional; only affects -# the dockerfile fallback (step 3 above). # REPO_PREFIX Path the consumer expects values to live under (default: "code"). # SKYHOOK_FILE Optional override for the config file path # (default: "$PWD/.skyhook/skyhook.yaml"). @@ -38,7 +40,6 @@ set -euo pipefail : "${SERVICE_NAME:=}" -: "${SERVICE_DIR:=}" : "${REPO_PREFIX:=code}" : "${SKYHOOK_FILE:=.skyhook/skyhook.yaml}" @@ -82,6 +83,7 @@ ROOT_DFP="" SVC_CTX="" SVC_CTX_LEGACY="" SVC_DFP="" +SVC_PATH="" CONFIG_PRESENT=0 if [[ -f "$SKYHOOK_FILE" ]]; then @@ -99,11 +101,12 @@ if [[ -f "$SKYHOOK_FILE" ]]; then SVC_CTX=$(yq_get '(.services // []) | map(select(.name == strenv(SERVICE_NAME))) | (.[0].buildTool.docker.buildContext // "")' "$SKYHOOK_FILE") SVC_CTX_LEGACY=$(yq_get '(.services // []) | map(select(.name == strenv(SERVICE_NAME))) | (.[0].buildTool.docker.contextPath // "")' "$SKYHOOK_FILE") SVC_DFP=$(yq_get '(.services // []) | map(select(.name == strenv(SERVICE_NAME))) | (.[0].buildTool.docker.dockerfilePath // "")' "$SKYHOOK_FILE") + SVC_PATH=$(yq_get '(.services // []) | map(select(.name == strenv(SERVICE_NAME))) | (.[0].path // "")' "$SKYHOOK_FILE") else log "::warning::Service '$SERVICE_NAME' not found in $SKYHOOK_FILE; only root-level overrides (if any) will apply." fi else - log "::warning::service_name '$SERVICE_NAME' was provided but $SKYHOOK_FILE was not found; using SERVICE_DIR / code defaults." + log "::warning::service_name '$SERVICE_NAME' was provided but $SKYHOOK_FILE was not found; using code defaults." fi # One-shot deprecation warning if anyone is still on `contextPath` AND the @@ -126,12 +129,13 @@ log "Using context: $RESOLVED_CONTEXT" emit "resolved_context=$RESOLVED_CONTEXT" # ── Dockerfile chain ──────────────────────────────────────────────────────── -# per-service > root > /Dockerfile > Dockerfile. +# per-service > root > /Dockerfile > Dockerfile. +# `service.path` is read from skyhook.yaml — no separate input needed. DFP="${SVC_DFP:-$ROOT_DFP}" if [[ -n "$DFP" ]]; then RESOLVED_DOCKERFILE="$REPO_PREFIX/$DFP" -elif [[ -n "$SERVICE_DIR" ]]; then - RESOLVED_DOCKERFILE="$REPO_PREFIX/$SERVICE_DIR/Dockerfile" +elif [[ -n "$SVC_PATH" ]]; then + RESOLVED_DOCKERFILE="$REPO_PREFIX/$SVC_PATH/Dockerfile" else RESOLVED_DOCKERFILE="$REPO_PREFIX/Dockerfile" fi diff --git a/test/unit/fixtures/no-buildtool-anywhere.yaml b/test/unit/fixtures/no-buildtool-anywhere.yaml index 256b7de..ab96d27 100644 --- a/test/unit/fixtures/no-buildtool-anywhere.yaml +++ b/test/unit/fixtures/no-buildtool-anywhere.yaml @@ -1,3 +1,7 @@ services: - - name: bare + # Service has `path` set — used as the SERVICE_DIR fallback for dockerfile. + - name: bare-with-path path: apps/bare + + # Service has no `path` — dockerfile falls all the way through to code/Dockerfile. + - name: bare-no-path diff --git a/test/unit/test_resolve_skyhook_config.sh b/test/unit/test_resolve_skyhook_config.sh index 62b5462..5625823 100755 --- a/test/unit/test_resolve_skyhook_config.sh +++ b/test/unit/test_resolve_skyhook_config.sh @@ -31,13 +31,13 @@ fi pass=0; fail=0 -# run_case +# run_case # # expected_outputs is a `;`-separated list of grep-E patterns that must each # match a line in $GITHUB_OUTPUT. Prefix a pattern with `!` to assert it MUST # NOT match. run_case() { - local name=$1 fixture=$2 svc=$3 svc_dir=$4 expects=$5 + local name=$1 fixture=$2 svc=$3 expects=$4 local case_dir out_file case_dir="$TMPROOT/$name" mkdir -p "$case_dir" @@ -45,7 +45,6 @@ run_case() { : > "$out_file" SERVICE_NAME="$svc" \ - SERVICE_DIR="$svc_dir" \ REPO_PREFIX="code" \ SKYHOOK_FILE="$FIXTURES/$fixture" \ GITHUB_OUTPUT="$out_file" \ @@ -88,57 +87,58 @@ echo "running unit tests against $RESOLVER" # 1. Canonical buildContext + dockerfilePath, both per-service. run_case "canonical-fields-emitted" \ - "canonical-only.yaml" "web" "" \ + "canonical-only.yaml" "web" \ "^resolved_context=code/apps/web/src$ ; ^resolved_dockerfile=code/apps/web/docker/Dockerfile$" # 2. Legacy contextPath honored (back-compat path). When dockerfilePath is -# unset, dockerfile falls all the way through to `code/Dockerfile` because -# no SERVICE_DIR was passed (NOT to `code//Dockerfile`; context -# and dockerfile chains are independent). +# unset, dockerfile falls back to /Dockerfile (NOT to +# /Dockerfile; context and dockerfile chains are independent). run_case "legacy-contextpath-honored" \ - "legacy-contextpath.yaml" "legacy" "" \ - "^resolved_context=code/apps/legacy$ ; ^resolved_dockerfile=code/Dockerfile$" + "legacy-contextpath.yaml" "legacy" \ + "^resolved_context=code/apps/legacy$ ; ^resolved_dockerfile=code/apps/legacy/Dockerfile$" # 3. Per-service value overrides root. run_case "service-overrides-root" \ - "root-and-services.yaml" "web" "" \ + "root-and-services.yaml" "web" \ "^resolved_context=code/apps/web/src$ ; !^resolved_context=code/shared$" # 4. Service with no per-service buildTool inherits root buildContext / dockerfilePath. run_case "service-inherits-root" \ - "root-and-services.yaml" "api" "" \ + "root-and-services.yaml" "api" \ "^resolved_context=code/shared$ ; ^resolved_dockerfile=code/shared/Dockerfile$" # ── Fallback chain (no YAML override) ────────────────────────────────────── -# 5. No buildTool anywhere, no SERVICE_DIR → context=code, dockerfile=code/Dockerfile. -run_case "no-override-no-service-dir" \ - "no-buildtool-anywhere.yaml" "bare" "" \ - "^resolved_context=code$ ; ^resolved_dockerfile=code/Dockerfile$" - -# 6. No buildTool anywhere, SERVICE_DIR set → context=code, dockerfile=code//Dockerfile. -run_case "no-override-with-service-dir" \ - "no-buildtool-anywhere.yaml" "bare" "apps/bare" \ +# 5. No buildTool anywhere, but service has `path` in YAML → context=code, +# dockerfile=code//Dockerfile. +run_case "no-override-with-service-path" \ + "no-buildtool-anywhere.yaml" "bare-with-path" \ "^resolved_context=code$ ; ^resolved_dockerfile=code/apps/bare/Dockerfile$" -# 7. `./` is normalized to "no override" — falls through to defaults. -# (dockerfilePath in the fixture is set, so dockerfile uses that.) +# 6. No buildTool AND no service.path in YAML → context=code, dockerfile=code/Dockerfile. +run_case "no-override-no-service-path" \ + "no-buildtool-anywhere.yaml" "bare-no-path" \ + "^resolved_context=code$ ; ^resolved_dockerfile=code/Dockerfile$" + +# 7. `./` is normalized to "no override" — context falls through to default. +# (dockerfilePath in the fixture is explicitly set, so dockerfile uses that.) run_case "dotslash-context-is-normalized" \ - "dotslash-context.yaml" "monorepo-root" "apps/foo" \ + "dotslash-context.yaml" "monorepo-root" \ "^resolved_context=code$ ; ^resolved_dockerfile=code/apps/foo/Dockerfile$" # ── No-op edge cases ─────────────────────────────────────────────────────── # 8. Empty service_name → manual mode, no outputs at all. run_case "no-service-name-is-noop" \ - "canonical-only.yaml" "" "" \ + "canonical-only.yaml" "" \ "!^resolved_context= ; !^resolved_dockerfile= ; !^config_file= ; !^service_name=" # 9. Service not found in YAML → root-level overrides (none in this fixture) -# + SERVICE_DIR-based dockerfile fallback. Diagnostic warning emitted. +# apply, no service.path is available, so dockerfile falls all the way +# through to code/Dockerfile. Diagnostic warning emitted (not asserted here). run_case "unknown-service-uses-defaults" \ - "canonical-only.yaml" "does-not-exist" "apps/missing" \ - "^resolved_context=code$ ; ^resolved_dockerfile=code/apps/missing/Dockerfile$ ; ^service_name=does-not-exist$" + "canonical-only.yaml" "does-not-exist" \ + "^resolved_context=code$ ; ^resolved_dockerfile=code/Dockerfile$ ; ^service_name=does-not-exist$" echo if [[ "$fail" -eq 0 ]]; then From 87f5e5f918609b7eed679ed488707e7da66e07aa Mon Sep 17 00:00:00 2001 From: eliran-mic Date: Thu, 30 Apr 2026 14:12:02 +0300 Subject: [PATCH 4/4] docs(skyhook): document config mode + surface yq parse errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit README had zero docs for skyhook config mode, so users hitting the new `contextPath` deprecation warning landed on a 404 anchor and had no way to learn the resolution chain or override semantics. Resolver also silently swallowed yq stderr, hiding malformed YAML behind a "no overrides" fallback that looked identical to a clean config. - README: new `## Skyhook Config` section (schema, resolution chain, override semantics, deprecated alias) + `service_name` input row + example. Anchor matches the URL emitted by the deprecation warning. - Resolver: drop `2>/dev/null` from yq calls so parse errors land in the job log; keep `|| true` so the chain still falls through. - Tests: assert the new stderr-surfacing contract on a malformed fixture, and lock the README↔resolver anchor consistency so the warning link can't silently rot in a future docs rewrite. Made-with: Cursor --- README.md | 95 ++++++++++++++++++++++++ scripts/resolve_skyhook_config.sh | 11 ++- test/unit/fixtures/malformed.yaml | 2 + test/unit/test_resolve_skyhook_config.sh | 49 +++++++++++- 4 files changed, 151 insertions(+), 6 deletions(-) create mode 100644 test/unit/fixtures/malformed.yaml diff --git a/README.md b/README.md index 13d4a63..21c45ff 100644 --- a/README.md +++ b/README.md @@ -103,6 +103,11 @@ The simplest way to use this action - just provide the image repository and prim *Must provide either (`image` + `base_tag`) OR `tags` +#### Skyhook Config (auto-resolve context + Dockerfile) +| Input | Description | Required | Default | +|-------|-------------|----------|---------| +| `service_name` | Name of a service defined in `.skyhook/skyhook.yaml`. When set, `context` and `dockerfile` are resolved from that file (see [Skyhook Config](#skyhook-config) below). | No | - | + ### Build Configuration | Input | Description | Required | Default | @@ -169,6 +174,71 @@ All parameters prefixed with `buildx_` are passed directly to docker/setup-build | `metadata` | Build result metadata | | `tags_list` | Newline-delimited list of image:tag combinations | +## Skyhook Config + +When `service_name` is set, the action reads `.skyhook/skyhook.yaml` from the **calling workflow's checkout directory** (which must live under `code/` — i.e. `actions/checkout` is expected to have placed the source under `./code`) and uses it to derive the build context and Dockerfile. The explicit `context` / `dockerfile` inputs are ignored in this mode. + +### Schema + +The action looks at `buildTool.docker.{buildContext,dockerfilePath}` at two levels — root (applies to every service) and per-service (overrides the root): + +```yaml +# .skyhook/skyhook.yaml +buildTool: + docker: + buildContext: shared # root-level default for every service + dockerfilePath: shared/Dockerfile + +services: + - name: api + path: apps/api # used as a fallback for Dockerfile only + + - name: worker + path: apps/worker + buildTool: + docker: + buildContext: apps/worker/src # per-service override + dockerfilePath: apps/worker/docker/Dockerfile +``` + +All paths are repo-root-relative. `.` and `./` are normalised to "no override" — use them when you mean "fall through to the next step in the chain". + +### Resolution chain + +| step | context | dockerfile | +|-------|-------------------------------|-----------------------------------------------| +| 1 | per-service `buildContext` | per-service `dockerfilePath` | +| 2 | root `buildContext` | root `dockerfilePath` | +| 3 | _(no further fallback)_ | `/Dockerfile` | +| 4 | `code` (entire checkout) | `code/Dockerfile` | + +The two chains are independent: setting only `buildContext` does **not** make `dockerfile` resolve relative to it — `dockerfile` runs through its own chain. + +### Override semantics (important) + +Once `service_name` is set, the resolved values **always take precedence** over the action's own `context` / `dockerfile` inputs — even when no override is found in YAML and the chain falls through to `code` / `code/Dockerfile`. This is intentional so behaviour is predictable across the matrix of "service exists with overrides", "service exists without overrides", "service not found", and "no `.skyhook/skyhook.yaml` at all". + +If you want the calling workflow's `context` / `dockerfile` inputs to be honoured, **don't set `service_name`** (manual mode). + +### Deprecated field: `contextPath` + +`buildTool.docker.contextPath` is the legacy alias for `buildContext`. It is still honoured for backwards compatibility, but the action emits a one-shot warning and you should rename it. `dockerfilePath` has no historical alias. + +### Example + +```yaml +- uses: actions/checkout@v4 + with: + path: code # required: action expects sources under ./code + +- uses: skyhook-io/docker-build-push-action@v1 + with: + image: ghcr.io/${{ github.repository }} + base_tag: v1.2.3 + service_name: worker # everything else (context, dockerfile) + # comes from .skyhook/skyhook.yaml +``` + ## Examples ### Using Automatic Tag Generation @@ -233,6 +303,31 @@ All parameters prefixed with `buildx_` are passed directly to docker/setup-build push: true ``` +### Skyhook Config Mode + +```yaml +# .skyhook/skyhook.yaml in your repo: +# services: +# - name: api +# path: apps/api +# buildTool: +# docker: +# buildContext: apps/api +# dockerfilePath: apps/api/Dockerfile + +- uses: actions/checkout@v4 + with: + path: code + +- uses: skyhook-io/docker-build-push-action@v1 + with: + image: ghcr.io/${{ github.repository }} + base_tag: v1.2.3 + service_name: api # context + dockerfile come from .skyhook/skyhook.yaml +``` + +See [Skyhook Config](#skyhook-config) for the full schema, resolution chain, and override semantics. + ### Build with Build Arguments ```yaml diff --git a/scripts/resolve_skyhook_config.sh b/scripts/resolve_skyhook_config.sh index 50c81ca..5235e14 100755 --- a/scripts/resolve_skyhook_config.sh +++ b/scripts/resolve_skyhook_config.sh @@ -29,7 +29,8 @@ # are used as-is). # REPO_PREFIX Path the consumer expects values to live under (default: "code"). # SKYHOOK_FILE Optional override for the config file path -# (default: "$PWD/.skyhook/skyhook.yaml"). +# (default: ".skyhook/skyhook.yaml", resolved relative to +# the working directory the action runs the script in). # GITHUB_OUTPUT File to append `key=value` outputs to (GHA-compatible). # If unset, outputs go to stdout instead. # @@ -54,9 +55,13 @@ emit() { log() { printf '%s\n' "$*" >&2; } # yq_get — returns "" for missing/null and normalizes "."/"./". +# Stderr from yq is intentionally NOT redirected: malformed YAML or yq errors +# should surface in the job log instead of silently falling through to +# defaults. `|| true` keeps the script alive on a non-zero exit so the +# resolution chain can still finish. yq_get() { local q=$1 file=$2 out - out=$(yq "$q" "$file" 2>/dev/null || true) + out=$(yq "$q" "$file" || true) # yq emits the literal string "null" when a path resolves to a missing key # and `// ""` did not absorb it (e.g. when a parent path is itself absent). [[ "$out" == "null" ]] && out="" @@ -94,7 +99,7 @@ if [[ -f "$SKYHOOK_FILE" ]]; then ROOT_CTX_LEGACY=$(yq_get '.buildTool.docker.contextPath // ""' "$SKYHOOK_FILE") ROOT_DFP=$(yq_get '.buildTool.docker.dockerfilePath // ""' "$SKYHOOK_FILE") - SERVICE_EXISTS=$(yq '(.services // []) | map(select(.name == strenv(SERVICE_NAME))) | .[0].name // ""' "$SKYHOOK_FILE" 2>/dev/null || true) + SERVICE_EXISTS=$(yq '(.services // []) | map(select(.name == strenv(SERVICE_NAME))) | .[0].name // ""' "$SKYHOOK_FILE" || true) [[ "$SERVICE_EXISTS" == "null" ]] && SERVICE_EXISTS="" if [[ -n "$SERVICE_EXISTS" ]]; then log "Found service '$SERVICE_NAME' in config" diff --git a/test/unit/fixtures/malformed.yaml b/test/unit/fixtures/malformed.yaml new file mode 100644 index 0000000..3591404 --- /dev/null +++ b/test/unit/fixtures/malformed.yaml @@ -0,0 +1,2 @@ +::: not yaml ::: +- "[unterminated diff --git a/test/unit/test_resolve_skyhook_config.sh b/test/unit/test_resolve_skyhook_config.sh index 5625823..cf0c3a1 100755 --- a/test/unit/test_resolve_skyhook_config.sh +++ b/test/unit/test_resolve_skyhook_config.sh @@ -31,13 +31,15 @@ fi pass=0; fail=0 -# run_case +# run_case [expected_stderr_pattern] # # expected_outputs is a `;`-separated list of grep-E patterns that must each # match a line in $GITHUB_OUTPUT. Prefix a pattern with `!` to assert it MUST -# NOT match. +# NOT match. expected_stderr_pattern, if non-empty, must match somewhere in +# the resolver's stderr (used to assert that errors/warnings actually surface +# to the job log instead of being silently swallowed). run_case() { - local name=$1 fixture=$2 svc=$3 expects=$4 + local name=$1 fixture=$2 svc=$3 expects=$4 expect_stderr=${5:-} local case_dir out_file case_dir="$TMPROOT/$name" mkdir -p "$case_dir" @@ -68,6 +70,12 @@ run_case() { fi fi done + unset IFS + + if [[ -n "$expect_stderr" ]] && ! grep -Eq "$expect_stderr" "$case_dir/stderr"; then + printf ' FAIL %s: missing stderr line matching /%s/\n' "$name" "$expect_stderr" + ok=0 + fi if [[ "$ok" == "1" ]]; then printf ' ok %s\n' "$name" @@ -140,6 +148,41 @@ run_case "unknown-service-uses-defaults" \ "canonical-only.yaml" "does-not-exist" \ "^resolved_context=code$ ; ^resolved_dockerfile=code/Dockerfile$ ; ^service_name=does-not-exist$" +# 10. Malformed YAML must surface yq's parse error to stderr (i.e. to the GHA +# job log) instead of being silently swallowed. The resolver still falls +# through to defaults so the build can proceed — the contract is "noisy +# fallback", not "fail closed". +run_case "malformed-yaml-surfaces-error" \ + "malformed.yaml" "any" \ + "^resolved_context=code$ ; ^resolved_dockerfile=code/Dockerfile$" \ + "Error|error" + +# ── Docs-vs-code consistency ─────────────────────────────────────────────── + +# 11. The resolver's deprecation warning links to a `#skyhook-config` anchor +# in README.md. Make sure that anchor still exists — silent-rotting docs +# turn the warning into a 404 for every user hitting the legacy field. +readme_anchor_check() { + local readme="$REPO_ROOT/README.md" + local script="$RESOLVER" + local linked_anchor heading_slug + linked_anchor=$(grep -oE '#skyhook-config[^ )]*' "$script" | head -n1 || true) + if [[ -z "$linked_anchor" ]]; then + printf ' ok readme-anchor-for-deprecation-warning (no anchor referenced)\n' + pass=$((pass + 1)) + return + fi + # GitHub turns "## Skyhook Config" into anchor "#skyhook-config". + if grep -qE '^## +Skyhook Config *$' "$readme"; then + printf ' ok readme-anchor-for-deprecation-warning\n' + pass=$((pass + 1)) + else + printf ' FAIL readme-anchor-for-deprecation-warning: %s referenced from resolver but no matching `## Skyhook Config` heading in README.md\n' "$linked_anchor" + fail=$((fail + 1)) + fi +} +readme_anchor_check + echo if [[ "$fail" -eq 0 ]]; then echo "ALL $pass UNIT TESTS PASSED"