docs: record the repo gotchas a fresh session keeps rediscovering - #56
Merged
Conversation
Adds the context that a full defect audit of this package turned out to need, and that nothing in the code or git history conveys. The main one is a section on where the two test doubles diverge from a real Chef Server. Both suites go green on wire shapes that a real server would reject or shape differently, and each listed gap concealed a real bug: cinc-zero returning only all_files, accepting a null run_list, and populating a group's courtesy "name" field. Also records the invariants that are easy to break silently - the signed path must equal r.URL.EscapedPath(), identifiers are esc()'d, only wire failures are retriable, the exact-dynamic-type rule that makes a bare map[string]any assertion miss an Attributes value - and the golangci-lint invocation that produces signal on this package. Signed-off-by: Tim Smith <tim@mondoo.com>
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.
Adds the context a full defect audit of this package turned out to need, and that nothing in the code or git history conveys.
The main addition: where the test doubles lie
A new "What the test doubles do not cover" section. Both suites go green on wire shapes a real Chef Server would reject or shape differently, and every gap listed there actually concealed a bug found in this audit:
all_fileson a cookbook GET → hid the manifest duplication (fix: deduplicate cookbook manifest files by path #46)run_list: null→ hid the nil-slice encoding (fix: encode an empty run list as [] rather than null #50)namefield → hid theGroups.Updatedependency on it (fix: fall back to GroupName in Groups.Update #48)Plus the throwaway-probe technique that pinned these down, since "write a
zz_probe_test.go, log what the client sends vs. what the server receives, delete it" is not obvious from the repo.The rest
esc()every caller-supplied identifier; a create returns(*Response, error)when the server answers with{"uri":…}r.URL.EscapedPath(), notr.URL.Path(which is decoded and hides a mismatch), andverifySignatureexists to check it; onlytransportErr-marked failures retryrun_listandnormal.tags; a Go type assertion matches the dynamic type exactly, so a baremap[string]anyassertion missesAttributesgolangci-lintinvocation that gives signal here, whygosecis mostly noise, and that a barestaticcheckbinary fails on a Go-version mismatchDependency on the sibling PRs
Some lines describe code that lands with #44 (
esc), #45 (asAttributeMap), #50 (MarshalJSON), #51 (Createsignature) and #52 (transportErr). Merge this last, and if any of those is dropped, drop its line here — I've kept each to a single bullet so that's a one-line edit.Test plan