ci: fork-safe image builds, working chart lint, regenerated static manifests - #348
Merged
Conversation
…nerate static manifests Follow-ups from #346: - ci.yaml: fork PRs run with a read-only GITHUB_TOKEN, so every docker job failed with 'denied: installation not allowed to Write organization package'. Push jobs are now gated on the PR coming from this repository (and not dependabot); fork PRs instead build the images locally and hand them to the k8s integration job as workflow artifacts, keeping full test coverage without granting forks any registry access. - helm-chart.yaml: 'ct lint --chart-dirs charts' pointed at a directory that doesn't exist (the chart lives at operator/charts/netchecks), so chart linting never ran. Fix the paths, register the policy-reporter dependency repo, and fix the maintainer entry to a GitHub username as ct's --validate-maintainers requires. Chart bumped 0.4.0 -> 0.4.1 for the metadata change. - create-static-manifests.sh: build chart dependencies, render with crds.keep=false (the keep annotation is meaningless outside Helm and the sed left a dangling 'annotations:' key), and clean up the .bak file. - manifests/deploy.yaml: regenerated. It previously contained no CRDs at all (helm template skips the old crds/ directory) even though the docs present it as installing them; it now carries all three, plus current chart output. Claude-Session: https://claude.ai/code/session_01U71MHip9jr1QHckFbBL4Sp
Deploying netchecks-docs with
|
| Latest commit: |
11705d8
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://72484ae6.netchecks-docs.pages.dev |
| Branch Preview URL: | https://claude-pr-346-merge-review-4.netchecks-docs.pages.dev |
check-linters is failing on main and on every branch push: the archived chartboost/ruff-action installs the latest ruff, whose new rules flag 65 pre-existing findings that the repo's locked ruff (0.15.12, which passes cleanly) does not. Switch to the official astral-sh/ruff-action pinned to the locked version so lint results only change when the project chooses to upgrade ruff. Claude-Session: https://claude.ai/code/session_01U71MHip9jr1QHckFbBL4Sp
Coverage Report for CI Build 33508100448Coverage remained the same at 87.654%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
'Honours'/'honoured'/'behaviour' arrived in the chart comments via #346 and fail the typos check (locale en-us), which runs on push and is red on main for the same reason. Also exclude the generated operator/manifests/deploy.yaml from typos: it embeds upstream CRD descriptions from the same files the config already excludes. Claude-Session: https://claude.ai/code/session_01U71MHip9jr1QHckFbBL4Sp
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
The k8s integration job failed once it could finally reach the tests: the workflow's smoke install into the 'netchecks' namespace leaves the CRDs behind on uninstall (helm.sh/resource-policy: keep, the chart default since #346) with that release's ownership metadata, so the pytest fixture's install into a fresh netchecks-test-* namespace fails with 'invalid ownership metadata'. The two pytest sessions in the job would hit the same conflict with each other. Install with the chart's own crds.keep=false toggle in the smoke install and the test fixture, so every uninstall removes the CRDs and the next install starts clean. Production default is unchanged. Claude-Session: https://claude.ai/code/session_01U71MHip9jr1QHckFbBL4Sp
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
3 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.
Follow-ups from #346.
Fork PRs no longer need ghcr write access
Every docker job on #346 failed with
denied: installation not allowed to Write organization package— fork PRs run with a read-onlyGITHUB_TOKEN, butci.yamlpushed toghcr.iounconditionally, even onpull_request.The push path is untouched for pushes, releases, and same-repo PRs. For fork (and dependabot) PRs, gated by
github.event.pull_request.head.repo.full_name == github.repository:probe_docker/operator_dockerbuild the images locally (outputs: type=docker,dest=…tar) instead of pushing, and upload them as 1-day workflow artifacts.operator_docker_mergeis skipped (nothing was pushed, nothing to merge).k8sintegration job downloads the artifacts,docker loads them, andkind loads the samesha-…tags it previously pulled — so fork PRs keep full integration-test coverage without granting forks any registry access. Nopull_request_target, no secrets exposed to fork code.Chart linting actually runs now
helm-chart.yamlranct lint --chart-dirs chartsand filtered pushes onpaths: charts/**, but the chart lives atoperator/charts/netchecks— so the job has been finding zero charts and passing trivially. Fixed the paths, and two things that surfaced the moment linting became real:ctneeds thepolicy-reporterdependency repo registered (helm repo addstep).--validate-maintainersrequires the maintainernameto be a GitHub username; changedBrian Thorne→hardbyte(email/url unchanged). Chart bumped 0.4.0 → 0.4.1 for the metadata change, which also satisfies ct's version-bump check.Verified locally:
ct lint --target-branch=main --chart-dirs operator/chartspasses end to end (maintainer validation confirmed separately; this sandbox can't reach api.github.com).Static manifests regenerated
manifests/deploy.yamlcontained no CRDs at all (the oldcrds/directory was invisible tohelm template) and was rendered from a chart several versions old, while the installation docs present it as installing "the NetworkAssertion CRDs and the Netchecks operator". Regenerated from chart 0.4.1: it now carries all three CRDs plus current chart output.create-static-manifests.shnow builds chart dependencies itself, renders with--set crds.keep=false(thekeepannotation is meaningless outside Helm, and thesedcleanup left a danglingannotations:key without it), and removes its.bakleftovers. All 24 rendered documents parse as valid YAML.Noted, not fixed
ci.yaml:51referencesmatrix.python-version, which doesn't exist (the matrix key ispython), so the Coverallsflag-nameloses its Python suffix. Pre-existing and harmless; left alone to keep this focused.https://claude.ai/code/session_01U71MHip9jr1QHckFbBL4Sp
Generated by Claude Code