Repository navigation
Conversation
…se build [#9] Test workflow (lint, build, test, coverage report) and security workflow (govulncheck + Trivy image scan) were one ci.yml job set; split so each can be read, triggered and gated independently. Security workflow needs a dhi.io login step now: the Dockerfile's base images moved to Docker Hardened Images, so the CI image build 401s without it, same as a local build would. Both scan jobs also post their report as a PR comment before gating, so findings are visible without opening the Actions log. Added release.yml: builds and pushes to GHCR on a v*.*.* tag push, gated the same DHI login.
Dockerfile's build and runtime stages moved to dhi.io hardened images, so the image build and scan both need a dhi.io login first, or they 401 on the pull. MY_DHI_KEY is a combined base64 user:token auth blob (the same shape docker login writes to config.json), not a plain password, so docker/login-action's username/password inputs can't take it -- config.json is written directly instead. trivy-action was pinned to v0.28.0, whose release tag doesn't resolve (v-prefixed) and which pins aquasecurity/setup-trivy@v0.2.1 internally, a tag that no longer exists. v0.36.0 pins setup-trivy by commit SHA instead. govulncheck and Trivy now each get their own PR comment -- tagged with a hidden marker, shield header, run link and collapsible findings, matching beckn/beckn-discovr#456's report style -- instead of one combined report under a single tag. create-or-update-comment@v4 has no comment-tag input; passing one was silently ignored, so every run posted a fresh comment instead of updating one (PR #12 had three stacked coverage comments as proof). Each report is now looked up first via peter-evans/find-comment on its own marker and updated in place, falling through to a new comment only when none is found yet.
CLAUDE.md called for a threshold but none was set anywhere in the repo; 80 is the number given now. Parses the total% off go tool cover -func's last line and fails after the PR comment posts, so the number is visible before the job goes red. Current coverage is 68.6% -- this gate will fail until it's raised.
Image owner is now derived from github.repository_owner (lowercased, since GHCR refs must be lowercase and the context var keeps real case) instead of a hardcoded org name. A hardcoded name meant testing this workflow from a fork pushed straight into openagrinet/discovery-service's real package; deriving it makes the same file work unedited on a fork or on the org repo. GHCR login now tries a GHCR_PAT secret before falling back to GITHUB_TOKEN. GITHUB_TOKEN can be denied write_package when creating a brand-new package under a personal (non-org) namespace, even with this workflow's own packages: write and the repo's default token permissions both set to write -- a documented GHCR quirk. GHCR_PAT (a classic PAT with write:packages) is the workaround; GITHUB_TOKEN stays the default so an org-owned repo needs no extra secret.
…#9] Renamed release.yml to build-and-push.yml and split it into a build job (matrix over amd64/arm64, each native on its own runner, no QEMU) and a manifest job that stitches the two arch-specific pushes into one multi-arch tag via docker buildx imagetools create -- matching the build-and-push pattern already used elsewhere, so a pull resolves the right arch automatically instead of shipping amd64-only. MY_DHI_KEY (a combined base64 user:token blob written straight into config.json) is replaced with the standard split-credential form, DHI_USERNAME + DHI_PAT, authenticated through docker/login-action like the GHCR step already was -- one consistent login shape instead of two, and no more hand-written config.json. security.yml's dhi.io login moves to the same shape since it authenticates the same registry.
build-and-push.yml built and pushed straight through -- the image security.yml scans on a PR is a different build (untagged, no push) from the one that ends up in GHCR, so the artifact people actually pull was never scanned. Each arch's image is now built locally (load: true, no push), Trivy-gated at HIGH,CRITICAL (exit-code 1), and only pushed if that step passes -- a vulnerable image stops at the runner, not GHCR.
…ust one shape [#9] DHI_USERNAME/DHI_PAT assumed every dhi.io credential arrives as two plain values, but a docker login itself (or a token already minted as user:token) produces one combined base64 blob -- exactly config.json's own auth format. DHI_AUTH takes that blob directly when set; otherwise the two plain secrets are combined the same way docker login would. Either shape works without editing the workflow.
…ame [#9] Matches build-and-push.yml's naming -- the filename now says what the workflow does (lint, build, test, gate on coverage) instead of a bare noun. No behavioral change.
The raw govulncheck text dump (Vulnerability #/Module/Found in/Fixed in blocks) made a reviewer read line by line to see which module needs bumping to what. A module/installed/fixed/advisory table surfaces that at a glance; full raw output stays available in a collapsed section for anyone who wants it.
The pipe-table read fine on GitHub but looked inconsistent next to Trivy's own box-drawn report right above it in the same PR comment thread. Column widths are computed from content, same layout family (module/installed/fixed/advisory) Trivy already uses for the image scan, so both reports read the same way at a glance.
#9] The box table only carried module/version/advisory ID — someone unfamiliar with the module still had to open the full run log to learn what the vulnerability actually does or how to fix it. Each finding now gets its own Issue/Fix/Advisory block below the table, with a ready-to-run `go get <module>@<fixed> && go mod tidy` command.
A separate Issue/Fix/Advisory block per finding duplicated what the table already said and made short reports feel long. One Issue column (truncated) keeps the whole report a single glance-able table, same as Trivy's own.
Only the header had a divider before; Trivy's own table divides after each row too, so the two reports in the same PR thread now share one visual grammar.
Severity can't be added — checked govulncheck's own JSON (OSV records): Go's vulnerability DB carries no CVSS/severity rating, unlike Trivy's NVD-sourced OS/language CVEs, so there's no real data to put in that column. Matched Trivy's own multi-line-cell layout instead: the advisory link now wraps onto its own line inside the Issue cell, same as Trivy's Title column does for its CVE links, one column narrower and no duplicate ID that was already visible in the URL.
…#9] Two problems: a standalone Advisories line duplicated what the wrapped cell already showed, and truncation cut mid-word ("...can write outsi...") which read as broken rather than brief. Cut at the nearest word boundary, and drop the module-name prefix most summaries repeat from the Module column, so the Issue column reads clean at a fixed width — same single-table layout Trivy uses.
] Two real bugs surfaced on this branch's own CI run, not just style: stdlib findings print "Standard library" instead of "Module: ...", and the parser never reset its module variable per record — so both stdlib rows silently inherited the previous row's module name. A code-fenced ASCII table also can't render a link or wrap text, forcing mid-sentence truncation. A real markdown table fixes all three: parses whichever field format applies per record, and GitHub wraps and links normally without a manual width budget.
The previous pattern (v[0-9]+.[0-9]+.[0-9]+*) matched any suffix after the
patch number, including throwaway tags like v0.1.1-test used while testing
this workflow. Narrow the trigger to what's actually a release: a plain
v{SemVer} or a v{SemVer}-rcN pre-release, nothing else.
#9] GHCR is the only registry in use today, but hardcoding ghcr.io/ into every tag, login and push step meant adding a second registry later would mean editing every step. Gate each registry (GHCR, Docker Hub, ACR, Google Artifact Registry) behind an ENABLED repo variable instead: GHCR defaults on since it's what we use now, the rest default off and need no secrets until switched on. The image is still built once, scanned once with Trivy before anything is pushed, then re-tagged per enabled registry rather than rebuilt and rescanned per registry -- the release gate this workflow exists for doesn't get weaker by supporting more than one destination.
Both the step summary and the PR comment only ever showed tail -1 of coverage-func.txt -- the aggregate percentage, nothing else. Show go tool cover's full per-function output instead, so a reviewer can see which functions are actually under-covered rather than just the number that fails the gate.
govulncheck gave no severity for a finding (Go vulnerability DB carries none), which made triage guesswork. Trivy scans go.sum with the same NVD/GHSA severity data the image scan already uses, at the cost of losing reachability filtering: expect more findings, including ones the binary never calls. Mirrors the existing image scan structure: a report, a PR comment, a gate, this time scan-type fs against the repo instead of the built image. tools/ stays excluded from the scan for the same reason the image scan already excludes it -- that module is never linked into the shipped binary.
] Two problems with the previous PR-comment approach. First, it needed a hand-rolled table renderer (jq/awk) to turn Trivy's ASCII table into markdown, which is structure this workflow shouldn't have to define or maintain — GitHub's own code-scanning UI already renders SARIF, with diff annotations the comment approach never had. Second, the gate rescanned both targets a third and fourth time to decide pass/fail; Trivy refreshes its vulnerability DB on every invocation, so a rescan can disagree with the report it's supposedly gating on. The gate now counts results already sitting in the two uploaded SARIF files, so the Security tab and the pass/fail decision are the same answer by construction. limit-severities-for-sarif is required for the SARIF to obey `severity: HIGH,CRITICAL` at all — without it every severity Trivy knows lands in the report, and the gate would need its own filtering logic to match. govulncheck is gone from this job. Its advisory feed carries no severity field, so "fail on HIGH/CRITICAL" was never expressible against its output — it could only report, never gate. Coverage this drops (call-graph reachability, and Go stdlib CVEs against the toolchain version pinned in go.mod) is a known, accepted gap, not a blind spot: verified locally that the one real finding it ever caught (github.com/moby/go-archive, GO-2026-6253 / CVE-2026-17106) is identical to what Trivy's dependency scan reports as CVE-2026-17106.
…erage [#9] go test's own output has no per-test summary to build a report from, which is why the previous coverage step ran the whole suite a second time under make cover just to get a profile - same tests, paid for twice. gotestsum's format pkgname gives one line per package with its coverage inline and full output for failures only, from the same run that produces the coverage profile. The github-actions format was tried first and dropped: gotestsum drives go test in verbose JSON mode, so every per-test format also prints PASSING tests' output - and the acceptance suite logs a spec-fetch warning on every test, which turned into 755 collapsible log groups of mostly-identical warnings. pkgname does not have that failure mode. go-test-coverage replaces the raw per-function coverage dump for the same reason: that dump lists all ~550 functions regardless of coverage, where go-test-coverage leads with the total and pass/fail and then lists only the files with gaps - 59 lines instead of 550 on this repo. The step runs with continue-on-error so a failing threshold still produces a report; the last step turns that outcome back into a hard failure. fromJSON on the report output needed a fallback for the case the coverage step fails before producing one - an unguarded call would fail the job with a template error instead of a coverage message.
continue-on-error on the make lint step: the job still runs and still reports its real pass or fail state, but its conclusion is always success, so it can never gate a merge - including under a future required-status-checks rule someone adds without re-reading this file. CLAUDE.md keeps CI lint as the safety net behind the pre-commit hook specifically because that hook can be bypassed on the command line; this does not remove the safety net, only its ability to block. A red lint check should be a reason to look, never a reason a PR cannot merge - unlike the security job, whose gate stays hard.
Test coverage: 68% (min 80%) — failedFiles below 80%:
|
A fork PR carries a read-only GITHUB_TOKEN by default, so create-or-update-comment 403s there even with pull-requests: write declared on the job - failing the whole test job over a comment that was never going to post. continue-on-error on both comment steps: the actual gate is the coverage-check step above them, unaffected. From code review on PR #15.
nisargabd
force-pushed
the
chore/9-ci-cd-workflows-recheck
branch
from
September 1, 2026 17:08
a746318 to
4b07b9a
Compare
#9] Coverage: go-test-coverage's own report output is the same tabwriter dump ReportForHuman prints to a terminal, JSON-wrapped as one string - not structured data, so consuming it verbatim just moved the dump into the PR comment. Built from coverage.out directly instead: the headline from the action's total-coverage output, and a real markdown table listing only the files below the threshold - 18 rows instead of 68, and no uncovered-line-range noise, which matters to whoever opens the file and not to a PR summary. Trivy: added the PR comment back for both scans, posted right after each report step - pass or fail, before the gate even runs - reading the same SARIF already produced for the code-scanning upload, so the comment and the Security tab can never disagree. Trivy's SARIF has no structured severity/version fields; each finding's detail lives as prose in message.text, so the table columns are pulled out of that text with jq capture. Kept inline in the workflow rather than a separate script file - two call sites do not need a third file to stay in sync. Both PR-comment steps get continue-on-error: a fork PR's default GITHUB_TOKEN is read-only and 403s there even with pull-requests: write declared; the gate is what decides pass or fail, not the comment.
capture(...)?.v is valid jq syntax on 1.8 (a workstation) but a syntax error on 1.7.1 (what the ubuntu-24.04 runner ships) - postfix ? chained straight into a field access parses differently across versions. That syntax error killed the whole job in 26s, before the image was even built, which is why no Trivy report showed up on the PR at all despite the comment logic being otherwise correct. Replaced with a def that wraps the capture in an explicit // against a plain object before the field access, which is unambiguous on both versions - verified against a real jq 1.7.1 binary, not assumed. Also added continue-on-error to both Assemble ... PR comment steps themselves, matching the Find/Comment steps below them: a report formatting step must never be able to take the whole job down again, jq bug or not - the gate at the bottom is what decides pass or fail.
🛡️ Trivy — Go dependency scan (HIGH,CRITICAL)
|
🛡️ Trivy — image scan (HIGH,CRITICAL)No HIGH or CRITICAL findings. |
Both stages hard-required dhi.io, so a local docker build, make docker or docker compose up --build without a Docker Hardened Images subscription just 401s. Parameterized both FROM lines behind BUILD_IMAGE/RUNTIME_IMAGE build-args, defaulting to the same dhi.io images CI already uses, so nothing changes for CI or for a shipped image. A local build without DHI credentials overrides both to public equivalents plus INSTALL_BUILD_TOOLS=true, since the public golang:alpine image lacks the gcc/g++/musl-dev/make the DHI -dev variant bundles and this build needs a C toolchain for h3-go's cgo binding regardless of which base runs it. Verified both paths build clean: the default (unchanged, DHI) and the public-image override with INSTALL_BUILD_TOOLS=true. From code review on PR #15.
Trivy's image-scan gate in build-and-push.yml caught this on a real tag push to a fork (v0.1.0-rc1): an SSH source-address restriction bypass in golang.org/x/crypto, pulled in transitively and actually linked into the shipped binary - unlike moby/go-archive, which stays unbumped on purpose since it is test-only and never reaches the image. v0.55.0 fixes it upstream. Scoped to this one bump only: lint, build and the full test suite all still pass, confirmed locally before pushing.
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
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
Fresh branch/PR off the same content as #15, opened to get a clean CI run to
review without #15's earlier review-round noise. Same 5 files:
.github/workflows/{run-tests,security,build-and-push,ci}.yml,Dockerfile.security.yml: govulncheck replaced by two Trivy scans (dependency graph +built image), both HIGH/CRITICAL findings reported as SARIF via
github/codeql-action/upload-sarif(Security tab + PR annotations) andgated once by reading those same SARIF files back - no rescan for the gate.
run-tests.yml: tests run throughgotestsum --format pkgname, coveragegated by
vladopajic/go-test-coverage, lint made non-blocking(
continue-on-error: true) per review feedback.build-and-push.yml: registry-agnostic behind repo-variable flags, tagtrigger restricted to plain SemVer or an
-rcNsuffix.Why
#15 accumulated review rounds and force-pushes on top of an earlier CI
buildout; this branch/PR gives a single clean run of the current state to
verify against before continuing review there.
Testing
make lint,make build,make testall pass locally.go.mod/Dockerfile;gate correctly fails on the known, unfixed
moby/go-archiveandgolang.org/x/cryptoCVEs (left in place on purpose - proves the gateworks before either gets bumped).
Closes #9