Linux CI + Dockerfile + nightly container image (#1804 stage 3) - #2049
Conversation
A checked-in Darling/Dockerfile builds from source into the official aspnet image (which sets the DOTNET_RUNNING_IN_CONTAINER marker the stage-2 gate keys on, and gains libgssapi-krb5-2 — SqlClient probes it at connect time on Linux even for SQL auth). build.yml gains a path-filtered darling-linux job (publish linux-x64 + image build on every Darling PR); nightly gains a linux job uploading the linux-x64 tar.gz with its own checksum file and pushing ghcr :nightly. Validated end-to-end locally first: the containerized service against dockerized TimescaleDB + SQL Server 2022 ran 25 collectors green with an env:-referenced SQL password and both HTTP gates correct through mapped ports. That run also caught the worker's config-validation warning still claiming mcp.network.* is IGNORED in containers while the host correctly exposed it — fixed here with a container check. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| gate), so this notice would be a lie there — the smoke test caught it warning IGNORED in | ||
| the same breath as 'Starting MCP server on 0.0.0.0'. The postgres.network notice above | ||
| stays: the bundled store never runs in BYO mode, container or not. */ | ||
| if (config.Mcp.Network?.IsConfigured == true && !Hosting.DarlingHostBinding.IsRunningInContainer) |
There was a problem hiding this comment.
GetNetworkStartupWarnings now calls the impure DarlingHostBinding.IsRunningInContainer (an environment-variable read) directly inside what its own doc comment (line 220) still calls a pure function ("Pure so a unit test asserts the returned strings..."). That claim is no longer true, and there's a real consequence: none of the three existing tests in DarlingWorkerTests.cs exercise the container branch, so this PR's actual fix — suppressing the false "IGNORED" notice in a container — has zero test coverage. GetNetworkStartupWarnings_ByoWithNetwork_WarnsBothSectionsIgnored still passes today only because DOTNET_RUNNING_IN_CONTAINER happens to be unset on the Windows test runners; if that ever changes, the test's mcp-warning assertion silently starts failing for the wrong reason.
Compare with the established pattern for exactly this problem elsewhere in the same PR/area: DarlingHostBinding.ResolveBind and DarlingWebHostService.ResolveWebBind(WebConfig, bool, bool? inContainer = null) both keep the impure container read at the call site and thread an inContainer parameter into the pure logic so it stays unit-testable. GetNetworkStartupWarnings should follow the same shape — take an inContainer (or bool isRunningInContainer) parameter, with the single caller at line 471 passing Hosting.DarlingHostBinding.IsRunningInContainer — and gain a test asserting the mcp warning is suppressed when inContainer: true.
| permissions: | ||
| contents: write | ||
| # #1804: the linux job pushes the nightly container image to ghcr. | ||
| packages: write |
There was a problem hiding this comment.
This adds packages: write at the workflow level, which grants it to every job in the file (redispatch, build/windows, darling-pg) even though only the new linux job pushes to ghcr. Contrast with build.yml's darling-linux job in this same PR, which scopes down to permissions: contents: read at the job level for least privilege. Consider moving packages: write (and contents: write, if not otherwise needed by the other jobs) to a job-level permissions: block under linux instead of broadening the workflow default — same reasoning darling-pg/darling-linux already apply elsewhere in this repo.
ReviewOverview: Stage 3 of #1804 — adds a checked-in Correctness / design:
Lite/Darling parity: No drift — this is Darling/Linux-only infrastructure, and the PR is explicit that the bundled zero-admin store and managed mode stay Windows-only by design, which matches how Lite already works. Nothing here touches Lite. Security: Docker image installs only CI/workflow conventions: No SQL changes in this PR, so the T-SQL style guide doesn't apply here. |
The mcp.network notice names the container path now (#1804 stage 3's live-caught fix); the pin follows, and documents why it still fires in the test process (no container marker) while a containerized run suppresses it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| - 'PerformanceMonitor.Common/**' | ||
| - 'PerformanceMonitor.Collectors/**' | ||
| - 'PerformanceMonitor.Analysis/**' |
There was a problem hiding this comment.
These three lines are missing the /!(*.md) extglob carve-out that every other area filter in this file uses — including the core filter's identical PerformanceMonitor.Common/**/!(*.md) etc. at lines 92-94, and Darling/**/!(*.md) two lines above this hunk.
The comment block above the core filter (lines ~74-88) documents exactly why this matters, with a cited incident: a bare dir/** pattern (no markdown carve-out) makes the filter true for any file under that directory, including docs-only edits — proven in PR #1714 (a .gitignore-only change triggered the full Darling PG suite). Here, a markdown-only edit under PerformanceMonitor.Common/, PerformanceMonitor.Collectors/, or PerformanceMonitor.Analysis/ will now unnecessarily trigger darling-linux's dotnet publish + docker build on every PR touching those dirs' docs.
Suggest matching the established convention:
| - 'PerformanceMonitor.Common/**' | |
| - 'PerformanceMonitor.Collectors/**' | |
| - 'PerformanceMonitor.Analysis/**' | |
| - 'PerformanceMonitor.Common/**/!(*.md)' | |
| - 'PerformanceMonitor.Collectors/**/!(*.md)' | |
| - 'PerformanceMonitor.Analysis/**/!(*.md)' |
| gate), so this notice would be a lie there — the smoke test caught it warning IGNORED in | ||
| the same breath as 'Starting MCP server on 0.0.0.0'. The postgres.network notice above | ||
| stays: the bundled store never runs in BYO mode, container or not. */ | ||
| if (config.Mcp.Network?.IsConfigured == true && !Hosting.DarlingHostBinding.IsRunningInContainer) |
There was a problem hiding this comment.
This new suppression branch reads DarlingHostBinding.IsRunningInContainer (an impure environment read) directly inside GetNetworkStartupWarnings, and no test in DarlingWorkerTests.cs sets DOTNET_RUNNING_IN_CONTAINER to exercise the suppressed-in-a-container path — only the pre-existing "outside a container" branch is pinned (the modified test at DarlingWorkerTests.cs:182).
This is also a departure from the pattern DarlingHostBinding.ResolveBind itself documents for exactly this reason (see its inContainer parameter and the doc comment on IsRunningInContainer: "IMPURE ... which is why it lives beside — never inside — the pure ... ladder: the callers read it once and pass it in, so the decision table stays testable without environment games"). GetNetworkStartupWarnings could take inContainer as a parameter the same way, letting a test assert the warning is suppressed without mutating process environment state.
Right now the container-suppression behavior is only verified by the manual live-container smoke described in the PR body, not by CI.
| Darling/artifacts | ||
| deprecated | ||
| docs | ||
| *.md |
There was a problem hiding this comment.
Since the build context is the raw filesystem (not git), .dockerignore doesn't inherit .gitignore's exclusions — and the root .gitignore explicitly calls out appsettings.json as "Sensitive configuration (connection strings)" that's never committed but commonly present in a dev's working tree. The Dockerfile's COPY . . (repo root context) would pick up any such locally-present, git-ignored secret-bearing file on a plain local docker build, and since the service project is Sdk="Microsoft.NET.Sdk.Worker", a same-directory appsettings*.json is copied into the publish output too.
Worth mirroring the sensitive-file exclusions from .gitignore here (appsettings.json at minimum) as defense-in-depth, even though CI's checkout is always clean.
Review summaryStage 3 of #1804 — Linux CI job for the Darling service, checked-in Correctness of the one code fix ( CI filter drift: the new Security (defense-in-depth, not CI-exploitable): Minor/optional: Everything else — job gating/skip-on-release logic, the |
Summary
Stage 3 of #1804, per the plan on the issue. The service is cross-platform .NET on purpose, but nothing PROVED it — the linux-x64 publish and the container image built at release time or never.
What changed
Darling/Dockerfile(checked in, repo-root context): sdk-stage publish into the official aspnet runtime image — which sets theDOTNET_RUNNING_IN_CONTAINERmarker the stage-2 bind gate keys on — pluslibgssapi-krb5-2, which Microsoft.Data.SqlClient probes at connect time on Linux even for SQL auth. Found the hard way: the first container smoke's SQL connect died on the missing library..dockerignorekeeps the context lean.darling-linux: ubuntu, path-filtered exactly likedarling-pg(always reports a result, so it can be a required check; the filter includes this workflow so this PR validates the job on itself). It answers two questions per Darling PR: does the service still publish for linux-x64, and does the image still build. No tests run there — the test projects are net10.0-windows by construction; the pinned behavior runs on the Windows jobs.linux(needs: build, so the release exists and a no-new-commits night skips it too): uploadsPerformanceMonitorDarling-linux-x64-*.tar.gzwith its ownSHA256SUMS-linux.txt(the Windows job ownsSHA256SUMS.txt; a cross-job rewrite of one file is a race) and pushesghcr.io/erikdarlingdata/performancemonitor-darling:nightly+ the version tag (packages: writeadded).mcp.network.*is IGNORED undermanaged=falsewhile the containerized host correctly exposed it two lines later — it now checks the container marker, mirroring the stage-2 gate. The bundled pg-runtime is deliberately absent from the linux artifact: compose pairs the service with the official timescale/timescaledb image.Validation (local, end-to-end, before any CI round-trips)
The containerized service (arm64 twin of this exact Dockerfile) against dockerized TimescaleDB + SQL Server 2022:
env:reference (stage 1) — zero plaintext warnings;The workflow YAML parses (jobs: build, darling-pg, darling-linux / redispatch, check, build, darling-pg, linux); the new PR job proves itself on this PR's own checks.
🤖 Generated with Claude Code