Repository navigation
feat: verify this installer against the contracts of the repos it clones - #596
Closed
goldyfruit wants to merge 6 commits into
Closed
goldyfruit wants to merge 6 commits into
goldyfruit wants to merge 6 commits into
Conversation
The containers method builds nothing: it clones ovos-docker and hivemind-docker at a pinned ref and runs the compose files out of them. Four things are therefore an interface between those repositories and this one, and each has broken an install: the compose file names, the container names docker_container_exec targets, the variables the compose reads, and the images pulled. The third stays quiet. A compose file that starts reading a variable with an inline default keeps working and uses the fallback, so a value collected here is silently replaced - HIVEMIND_SITEID did exactly that and every satellite reported the wrong site with nothing failing. The contracts mark such a variable `owner: installer`, and this refuses to read "has a default" as "nobody needs to set it". Offline by default against the snapshots in tests/contracts/, so it runs in every pull request through code_quality.bats. --online re-fetches each contract at the ref pinned here and notices a snapshot that has drifted; --fail-on-stale additionally fails when a pin is behind a release, which is how an install sat on hivemind-docker v2.0.0 for weeks after the fix it needed had shipped. A scheduled job runs both, because a scheduled run that exits 0 tells nobody anything. Written against main first, where it immediately reported HIVEMIND_SITEID as unset - the bug this stack fixes - which is the case it exists for. Negative-tested by dropping an installer-owned variable, dropping a required variable, and renaming an expected compose file; each is reported by name, and the missing compose file correctly cascades into the container that is no longer reachable. The snapshots are derived from the compose files at the pinned tags, since contract.yml itself only lands in the next release of each repository; --online falls back to them and says so until then. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ansible-lint runs over the whole tree, yamllint included, and rejected the sequence indentation PyYAML emits by default. The generator in both upstream repositories now indents sequences and writes a document start; these snapshots are regenerated with it. Caught only in CI because ansible-lint 26.8.0 enforces this and the 26.1.1 I had did not - and because I had run it scoped to ansible/ rather than at the root the way CI does. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Comparing the pin against the newest release cannot see the failure that matters. Both can read v2.0.2 while ten days of fixes sit unreleased on the default branch - which is how the fann2 gating, the intent-engine assertion and the terminal client all failed to reach a single install while every check here was green. --online now also reports how far the newest release trails the branch it is cut from, and --fail-on-stale makes it a failure: ovos-docker: v2.0.2 is 24 commit(s) behind dev - those fixes are in no release, so no install has them Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
v2.1.0 and v2.1.1 ship contract.yml, so the snapshots stop being derived from the compose files at those tags and become what --online verifies them against. With the pins current, strict mode is quiet: no drift, no pin behind a release, and no release behind the branch it is cut from. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Collaborator
Author
|
Closed automatically when its base branch (#595) was merged and deleted; GitHub will not reopen a pull request whose base is gone. Continued as #597 from the same branch, based on 🤖 Generated with Claude Code |
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
1 participant
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.
Consumer half of a contract across the three repositories. Producer halves: ovos-docker#177, hivemind-docker#48.
Why
The containers method builds nothing. It clones ovos-docker and hivemind-docker at a pinned ref and runs the compose files out of them, which makes four things an interface — each of which has already broken an install:
v2.0.0for weeks after the fix shippeddocker_container_exectargets them; a rename fails late, after the stack is upThe last row is why this is not just "are all required variables set?".
HIVEMIND_SITEIDhas a fallback of the literal stringdefault, so an installer that never set it produced a working satellite reporting the wrong site, with nothing failing anywhere. The contracts mark such variablesowner: installer, and this refuses to read "has a default" as "nobody needs to set it".How it is checked
Offline against the snapshots in
tests/contracts/, throughcode_quality.bats— so every pull request runs it with no network.--onlinere-fetches each contract at the ref pinned here and reports a snapshot that has drifted.--fail-on-staleadditionally fails when a pin is behind a release. A scheduled weekly job runs both: a scheduled run that exits 0 tells nobody anything, so the stale pin has to be a failure to be an alert.It found the bug on its own
Written against
mainfirst, where it immediately reported:That is the bug this stack fixes, rediscovered independently by the check rather than encoded into it after the fact.
Negative tests
Case 3 cascading into case 3b is the behaviour I wanted: losing the compose file also loses the container the installer execs into, which is the
Could not find container "ovos_cli"failure mode.One ordering note
The snapshots are derived from the compose files at the pinned tags, because
contract.ymlitself only lands in the next release of each repository. Until then--onlinefalls back to the snapshot and says so:Once both producer PRs are released, the fetch takes over and the snapshots become a cache that
--onlineverifies rather than the source of truth.167/167 bats pass;
ansible-lintclean on the production profile.🤖 Generated with Claude Code