docs: refresh the repository documentation before it goes public (MK8S-391) - #18
Merged
Merged
Conversation
Cover the checklist for making a Scality repository public: a README that says what the project is, why it exists and how to start with either half, a CONTRIBUTING guide for the two toolchains the repository holds, a top-level DESIGN describing the split and the on-disk contract between the components, and a CODEOWNERS assigning reviews to a team. The agent's two scaffold files go with them. Its README was still the kubebuilder tutorial behind a banner announcing its own replacement, and its AGENTS.md documented webhooks, a multi-group layout and helm targets this project does not have. Both are the first page their respective readers land on. The agent keeps its own DESIGN.md: this one stays at the level of the project, and points at it. Every command, path and claim was checked against the code as it stands on main, and the docs say what the code does rather than what it ought to: - kubectl and kind are not downloaded by the Makefile, unlike the four tools that are, and the logcheck plugin resolves to latest rather than a pin; - the manifests do not label their namespace privileged, so deploying them as they are fails under a baseline or restricted default; - deploying needs the image built and pushed first, since none is published, and docker-build takes the architecture of the machine that runs it while the DaemonSet only schedules onto amd64; - undeploy takes the CRD with it, and every ImageCache object with that; - deploy applies the CRD too, so install before it is redundant, and it writes the image reference into a tracked file; - the package does not create the cache directory the walkthrough writes to; - the RPM filename carries a release field; - both halves target amd64, which the scope section claimed to make no assumption about; - the garbage collector also removes its own interrupted extractions, not only sentinel-bearing directories; - a cache path outside the default is forgotten when the process restarts; - resync-period=0 removes the fallback the watcher's own error path promises; - releases are not independent: a tag cuts one version for the repository and only the RPM is attached to it; - errors are wrapped with the sentinels of scality/go-errors rather than with fmt.Errorf, which .claude/REVIEW.md still told reviewers to expect. The resync period is documented where a user looks for it. Until now it only existed in the flag's own help text and in agent/DESIGN.md. The root README links to the agent's page for the detail instead of restating it. Two hazards are written down rather than fixed, because both are code changes: the unpinned logcheck plugin, and CI regenerating the manifests without failing on an uncommitted diff. Relates-to: MK8S-391
The criteria covered the code well and the docs barely: one row asking whether the docs were updated, none asking whether what they say is true. Two review passes on this branch spent most of their findings there, on commands that do not exist, a package that never creates the directory the walkthrough writes to, and a target documented as removing the DaemonSet that removes the CRD and every custom resource with it. Four rows now cover that ground: claims checked against what implements them, documented commands stating what they destroy, the doc set read as a set rather than file by file, and a footgun written down where a small change would remove it. There is also a section on how to report, since the same defect kept arriving as three findings and several named no line anyone could check. One finding per defect, the proof in the implementation rather than in the diff, the failure stated concretely, ranked by what it costs whoever hits it, and unchecked assumptions declared as such. The exclusions grow with it, so a reviewer stops re-opening what the commit message already argues. Relates-to: MK8S-391
Pull requests here have been free-form, so what a reviewer gets depends on who opened it. The sections are the ones reviews on this repository keep asking for anyway: which half the change touches, why it exists, what it decided, what was run, and what it deliberately leaves alone. The two checkboxes cover the failures a diff does not show. Docs drifting from the behaviour they describe is the one this branch spent most of its findings on. A cache layout change landing in one half only is what .claude/REVIEW.md calls a silent break. Relates-to: MK8S-391
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
AGENTS.md restated two paragraphs of CONTRIBUTING.md nearly word for word: the CI regeneration caveat, and why a green lint right after a dependency bump is not proof. Both now keep the action, which an agent reading this file needs on the spot, and link out for the reason. Caught by the criterion this branch adds two commits earlier: prefer a link to a second copy, since the copy is what goes stale. Relates-to: MK8S-391
ezekiel-alexrod
requested review from
TeddyAndrieux and
anthony-treuillier-scality
August 24, 2026 17:15
ezekiel-alexrod
added
the
documentation
The root README restated four paragraphs of agent/README.md: building for the right architecture, the privileged namespace, NODE_NAME, and what happens to a non-default cache path across a restart. Two pointers replace them. The walkthrough keeps what gets the happy path running and hands off the rest. Rechecking every page against the code turned up two claims in agent/DESIGN.md narrower than the implementation. `cachePath` also rejects `..`, and garbage collection removes the agent's own interrupted extractions next to the sentinel-bearing directories, which the same file already said twenty lines further down. Relates-to: MK8S-391
`agent/` had a README and `rpm/` had nothing, so the link from the root README landed a reader on a file listing. The page covers what the other half's page covers: installing, the two sysconfig knobs, what the glob picks up, and the toolchain. Three things were only readable in the sources. The package ships the units without enabling them, on purpose, since that call belongs to whatever provisions the node. It does not create the cache directory either. And the script runs under `set -euo pipefail`, so a tarball that fails to import ends that run and leaves the rest for the next tick. While writing it, one claim in DESIGN.md turned out to be mine and unmeasured: re-importing what containerd already has was said to cost a check. Idempotent is what was verified, so that is what both pages now say. Relates-to: MK8S-391
The deletion behaviour was stated in both READMEs, with the root carrying a qualifier the agent's page explains properly. Only the pointer stays. The YAML sample and the label query stay where they are. They are what using this project looks like, and a front page that shows none of it is worse than one block of overlap. Relates-to: MK8S-391
Nothing said where the image an ImageCache points at comes from. The README now shows it, since the answer is short: export the tarballs, put them in an empty image, push it. Three constraints deserve saying out loud, because none of them is guessable and each fails in a different way. The agent resolves linux/amd64 and no other platform. The image filesystem is flattened to base names, so two files sharing one fail the extraction rather than overwrite each other. And every regular file lands in the cache directory, so a conventional base image pours its whole filesystem in there. DESIGN.md kept building out of scope, which is still true of the tooling, but it described a cache image as any image whose layers contain tar exports. That is looser than what the code accepts. Relates-to: MK8S-391
TeddyAndrieux
approved these changes
Aug 26, 2026
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.
Component
docs, plus
.github/and.claude/.Problem
mainstill carries the two-line README from the repository skeleton, noCONTRIBUTING, no top-level DESIGN and no CODEOWNERS. The checklist for making a
repository public asks for all four.
A branch with that documentation was written in July but never pushed. It
branched off
mainbefore the agent landed, so the README it carried describedan
agent/directory the branch itself did not contain. The agent's own twofiles were no better: its README was still the kubebuilder tutorial behind a
banner announcing its own replacement, and
agent/AGENTS.mddocumentedwebhooks, a multi-group layout and helm targets this project does not have.
Both are the first page their respective readers land on.
Fix
Three commits.
The first replays the documentation on top of
mainand checks it against thecode rather than reusing it as is. Every command, path, filename, flag and
default was traced back to what implements it, and a fair number did not
survive:
make undeploydeletes the CRD, and Kubernetes garbage-collects everyImageCacheobject with it, becauseconfig/defaultincludes../crd.Nothing said so.
make deployalready applies the CRD, so runninginstallfirst isredundant, and it writes your registry into a tracked kustomization.
docker-buildtakes the architecture of the machine it runs on, while theDaemonSet only schedules onto amd64.
/var/lib/image-cache, which the walkthrough tells youto write into.
privileged, so they fail as theyare under a baseline or restricted default.
kubectlandkindare not downloaded by the Makefile, unlike the fourtools that are.
.claude/REVIEW.mdstill told reviewers to expectfmt.Errorf, which theagent stopped using when it moved to
scality/go-errors.The second sharpens
.claude/REVIEW.md. Its criteria covered the code well andthe docs barely, which is how the drift above lasted this long. Four rows now
cover claims checked against what implements them, documented commands stating
what they destroy, the doc set read as a set, and a footgun written down where
a small change would remove it. A section on reporting a finding goes with
them.
The third adds the pull request template this body follows.
Test
No code changed, so no test suite ran. What was checked instead: every relative
link resolves, the sample
ImageCachestill passes the CRD's CEL rule, andeach claim traced back to the file that implements it.
Out of scope
changes: the
logcheckplugin resolves tolatestin.custom-gcl.ymlwhile everything around it is pinned, and CI regenerates the manifests
without failing on an uncommitted diff. Each deserves its own ticket.
main.applicable: nothing here touches the layout.
Relates-to: MK8S-391