Repository navigation
ci: run manifest checks - #250
TomasArrachea wants to merge 20 commits into
Conversation
TomasArrachea
added
the
check:install
8cac007 to
5afc36d
Compare
bitwalker
left a comment
bitwalker
left a comment
There was a problem hiding this comment.
Looks good, just a couple of findings that we should fix first
| report(&format!("channel {}", channel.name), &channel.extra, errors); | ||
| for component in channel.components.iter() { | ||
| let at = format!("channel {}: component '{}'", channel.name, component.name); | ||
| report(&at, &component.extra, errors); |
There was a problem hiding this comment.
Unknown fields inside installation_method and version are discarded during deserialization, so checking component.extra cannot detect them. I added "featurs": ["nonexistent-review-feature"] inside an installation method; check exited successfully. A misspelled feature declaration can therefore silently change the build. Please preserve and validate nested unknown fields, with tests using actual JSON input.
| .execute_with_state(&config, &mut local_manifest) | ||
| .expect("failed to install stable"); | ||
| // mainnet is not available yet, so it is left out until it is. | ||
| let networks = ["devnet", "testnet"]; |
There was a problem hiding this comment.
Only devnet and testnet are tested. Mainnet currently shares testnet's toolchain, but an independent mainnet promotion—or adding another network—can publish an untested toolchain. Please derive the tested networks from the manifest; toolchain versions can be deduplicated if installation cost matters.
|
|
||
| // Last, because the rules above must hold before a comparison means anything. | ||
| if let Some(uri) = against { | ||
| let previous = VersionedManifest::load_from(uri) |
There was a problem hiding this comment.
VersionedManifest::load_from converts v1 manifests into v3 before comparison, losing the original schema version. I reproduced a v1.0.1 → v3.0.0 replacement passing check --against, despite the promised major-version safeguard. Please compare the original document headers before conversion. This affects legacy schema transitions; the current main and next manifests already use v3.
There was a problem hiding this comment.
I fixed this, but now I am actually not sure if we should have it. We should add an escape hatch or change it in a way that it lets us make new schema releases in the future.
There was a problem hiding this comment.
I replaced the header comparison with a CI step that checks directly that the latest midenup can read what gets published. A schema update would be a require two PRs, one for the midenup release and then the manifest update afterwards.
98fdea7 to
2b55ec8
Compare
bitwalker
left a comment
bitwalker
left a comment
There was a problem hiding this comment.
This looks good, with one exception around the handling of unknown fields. See the attached comment for details. There is also one documentation nit to address as well
| fn insert_at_path(object: &mut Extra, path: &str, value: &serde_json::Value) { | ||
| match path.split_once('.') { | ||
| None => { | ||
| object.entry(path.to_string()).or_insert_with(|| value.clone()); | ||
| }, | ||
| Some((head, rest)) => { | ||
| if let Some(serde_json::Value::Object(inner)) = object.get_mut(head) { | ||
| insert_at_path(inner, rest, value); |
There was a problem hiding this comment.
The new dotted-path representation loses or relocates unknown fields when serializing them back to JSON. I reproduced both cases:
- A literal top-level key such as
"vendor.flag": nullpasses validation but disappears when formatted, because the dot is interpreted as nesting. A literal"version.future": trueis instead relocated into theversionobject. - An unknown field inside the supported
installation-methodalias disappears:collect_unknownsaves the path using the alias, but serialization emits the canonicalinstallation_method, so the parent lookup here fails.
This undermines the promised preservation of unknown fields. Please preserve path segments structurally (for example, with nested extras) and canonicalize accepted alias roots. Regression tests should cover literal dotted keys and unknown nested fields under installation-method, including a parse/serialize round trip.
| cargo make check-manifest --against file:///tmp/previous-manifest.json | ||
| ``` | ||
|
|
||
| This refuses a timestamp that did not advance, a `manifest_version` major change, a removed network, |
There was a problem hiding this comment.
Documentation nit: check --against no longer rejects manifest_version major changes. Please remove that claim and describe the replacement CI check: the latest released midenup must be able to read the candidate manifest, so schema support ships in a binary release before the manifest starts using it.
|
I've been revisiting the overall approach here, and in addition to finding a few more issues, I've realized that we need a more principled approach to accomplish the actual goals we're after. We've kind of muddied the waters in general on what guarantees we're actually trying to provide and prove - and that's in large part because we haven't actually identified those guarantees and done the design work on how to properly uphold them. I'm going to follow up in #248 with my take on what guarantees we should try and provide, as well as how/when to check them. We can go from there on actual implementation, either by reworking this PR, or starting fresh - but for now let's put this on hold until we've discussed this a bit further. |
This branch has not been deployed
2 participants
Builds towards #248.
Hardens manifest releases with a comparison against the manifest it replaces, and a real install of every deployed toolchain. All checks run on
manifest/v3/manifest.json, the manifest current clients read. Changes:update-manifest checkin CI on every PR, on every push tonext, and before every manifest deploy.check --against <uri>, that compares the manifest with the previous one. Refuses a timestamp that did not advance, a removed network, a network moving to an older toolchain, a removed toolchain, a toolchain dropping or replacing themigrates_fromit was published with, and a lowermin_client_version.next, and against the deployed manifest before deploying. On a pull request, themanifest:allow-downgradelabel enables--allow-downgrade, and a failing downgrade check points to that label; the push and deploy checks allow downgrades.checknow rejects unknown fields at every level of the manifest, and unknown component kinds. The parser preserves them for forward compatibility, so a misspelled one would otherwise be published and silently never read. Explicitly empty values are exempt.checkrejects a component fetched from a local path or a git branch, since its contents change without the manifest changing, a component that cannot install on a target midenup is released for (unless it has a Cargo fallback), and a timestamp in the future.migrates_from: a toolchain may not declare itself, may only declare an older toolchain, and no two toolchains may declare the same predecessor.main, in addition to pushes tomainand labelled PRs. It now installs every toolchain a network names, not onlystable.clone-toolchainno longer copies the source'smigrates_from; a new--migrates-fromflag sets it explicitly.update-manifesttests to the CI.Pre-release tests are currently taking ~25m on the CI.