fix(skyhook): align resolver with canonical schema; stop overriding workflow inputs - #4
Closed
eliran-ops wants to merge 4 commits into
Closed
fix(skyhook): align resolver with canonical schema; stop overriding workflow inputs#4eliran-ops wants to merge 4 commits into
eliran-ops wants to merge 4 commits into
Conversation
…orkflow inputs 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
…file
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/<SERVICE_DIR>/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/<SERVICE_DIR>/Dockerfile (or code/Dockerfile) — it does NOT
auto-default to <context>/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
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/<service.path>/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
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
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 87f5e5f. Configure here.
| fi | ||
| } | ||
|
|
||
| log() { printf '%s\n' "$*" >&2; } |
There was a problem hiding this comment.
Workflow warning commands sent to stderr not stdout
Medium Severity
The log() helper writes to stderr (>&2), but all three ::warning:: workflow commands are routed through it. GitHub Actions only processes ::warning:: annotations from stdout — stderr output appears as plain text in the log without creating actual warning annotations in the UI. The old inline code used echo (stdout) for these warnings and they worked correctly. The deprecation warning, "service not found" warning, and "config file not found" warning will all fail to produce annotations.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 87f5e5f. Configure here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Fixes three independent bugs in the
Resolve build config from .skyhook/skyhook.yamlstep that combined to make the action silently overrideinputs.contextwithcodewheneverservice_namewas set, regardless of what the caller (or the YAML) actually configured.buildTool.docker.contextPath. The canonical schema (koala-backend/internal/conf/skyhook.go::SkyhookDockerBuild) usesbuildContext..services[]— no fallback to top-levelbuildTool.docker.*.resolved_context=code, and the consumer expression putresolved_contextfirst in the `Empirically observed on a real run (skyhook-dev/nbjkgj): the workflow correctly resolved
context: code/./srcfrom a root-levelbuildContext: ./src, the action then emittedUsing context: codeand overrode it, and the build failed with `buildx failed with: ERROR: failed to read dockerfile: open Dockerfile: no such file or directory`.How
Where to start reading
Out of scope
Test plan
Made with Cursor
Note
Medium Risk
Changes the action’s build context/Dockerfile resolution logic for
service_name, which can alter what gets built and where Dockerfile is read from. Added unit tests reduce regression risk, but mis-resolution would still break builds for existing users.Overview
Fixes Skyhook config mode so
service_nameresolves buildcontext/dockerfilefrom.skyhook/skyhook.yamlusing the canonicalbuildTool.docker.buildContext(with legacycontextPathsupport), root-vs-service precedence, and././normalization.The resolver is extracted from
action.ymlintoscripts/resolve_skyhook_config.sh, adds a deprecation warning forcontextPath, and consistently emitsresolved_context/resolved_dockerfile(including fallbacks viaservices[].path). CI now runs fast unit tests for this resolver, the test.skyhook/skyhook.yamlfixture is expanded to cover all branches, and the README documents the Skyhook config schema and resolution semantics.Reviewed by Cursor Bugbot for commit 87f5e5f. Bugbot is set up for automated code reviews on this repo. Configure here.