fix(release): fail when a package's versioned file does not exist - #1064
fix(release): fail when a package's versioned file does not exist#1064BryanFRD wants to merge 3 commits into
Conversation
SonarQube — aucune nouvelle issueComparaison entre le projet bac à sable de cette PR et la branche par défaut : SonarQube Community n'analyse pas les PR, ce delta est calculé côté CI. Détail |
There was a problem hiding this comment.
The direction is right and the scoping argument holds, but the check does not land where the description says it does: placed above the skip returns, it also fails packages the run would never write. One blocking finding on that, plus two nits with suggestions.
| tags_for_package(inputs.all_tags, &tag_search_prefix) | ||
| }); | ||
|
|
||
| ensure_versioned_files_exist(pkg, inputs.root)?; |
There was a problem hiding this comment.
Blocking: this sits above the NoNewCommits, NoReleasableCommits and VersionUnchanged returns, so it fails packages this run would not write, not just the ones it would.
Concretely: a package touched only by chore(api): bump lint config currently ends as Skipped { reason: NoReleasableCommits } and the release proceeds. After this change it returns Err, and one_package_compute_error_aborts_collection shows a single package's error aborts the whole collection. So one stale versionedFiles entry on a package nobody is releasing turns every future release into a hard failure, which is the same class of breakage the description gives as the reason for scoping the check.
Fix: move the call down to immediately before the final Ok(PackagePlan::Bump(...)). file_source already tolerates a missing file through .ok(), current_version then falls back to the tag, and the plan is refused at the point where it would actually tag and write, so the issue's repro still fails with the same message. A fourth test (touched package, chore-only commit, missing file, expect a skip rather than an error) would pin the new position.
| return Err(anyhow!( | ||
| "package \"{name}\": versioned file \"{path}\" does not exist, so this release \ | ||
| would create a tag no manifest carries.\n \ | ||
| Paths in versionedFiles are relative to the repository root, not to the \ | ||
| package's own path. Did you mean \"{suggestion}\"?", | ||
| name = pkg.name, | ||
| path = vf.path, | ||
| suggestion = suggested_versioned_path(pkg, &vf.path), | ||
| )) | ||
| .error_code(error_code::CONFIG_MISSING_VERSIONED_FILE); |
There was a problem hiding this comment.
Nit: the "Did you mean" clause is printed unconditionally, including when the suggestion is byte-identical to the path the user wrote. That is the majority case once this lands: a package at packages/api with versionedFiles: ["packages/api/Cargo.toml"] whose file was renamed or deleted gets versioned file "packages/api/Cargo.toml" does not exist [...] Did you mean "packages/api/Cargo.toml"?. Same for a root package (path = "."). Only print the hint when it says something new:
| return Err(anyhow!( | |
| "package \"{name}\": versioned file \"{path}\" does not exist, so this release \ | |
| would create a tag no manifest carries.\n \ | |
| Paths in versionedFiles are relative to the repository root, not to the \ | |
| package's own path. Did you mean \"{suggestion}\"?", | |
| name = pkg.name, | |
| path = vf.path, | |
| suggestion = suggested_versioned_path(pkg, &vf.path), | |
| )) | |
| .error_code(error_code::CONFIG_MISSING_VERSIONED_FILE); | |
| let suggestion = suggested_versioned_path(pkg, &vf.path); | |
| let hint = if suggestion == vf.path { | |
| String::new() | |
| } else { | |
| format!( | |
| "\n Paths in versionedFiles are relative to the repository root, not to \ | |
| the package's own path. Did you mean \"{suggestion}\"?" | |
| ) | |
| }; | |
| return Err(anyhow!( | |
| "package \"{name}\": versioned file \"{path}\" does not exist, so this release \ | |
| would create a tag no manifest carries.{hint}", | |
| name = pkg.name, | |
| path = vf.path, | |
| )) | |
| .error_code(error_code::CONFIG_MISSING_VERSIONED_FILE); |
The path.starts_with(prefix) test in suggested_versioned_path is also a string prefix rather than a path prefix, so package api matches apiv2/Cargo.toml and skips the suggestion. Harmless with the above, since the message then just drops the hint.
| "", | ||
| &format!( | ||
| r#"{{"name":"api","path":"{pkg_path}","versionedFiles":[{{"path":"{versioned_path}","format":"toml"}}]}}, | ||
| {{"name":"sdk","path":"sdk","versionedFiles":[{{"path":"sdk/Cargo.toml","format":"toml"}}]}}"# |
There was a problem hiding this comment.
Nit: an_untouched_package_is_skipped_before_its_files_are_checked has no teeth as written. write_pkg(&root, "sdk", "1.0.0") creates sdk/Cargo.toml, so sdk's versioned file exists and the test passes whether the check is scoped to touched packages or run repository-wide, which is the one thing it claims to pin. Point sdk at a file that is absent:
| {{"name":"sdk","path":"sdk","versionedFiles":[{{"path":"sdk/Cargo.toml","format":"toml"}}]}}"# | |
| {{"name":"sdk","path":"sdk","versionedFiles":[{{"path":"sdk/Missing.toml","format":"toml"}}]}}"# |
The other two tests only plan api, so they are unaffected.
There was a problem hiding this comment.
Follow-up on 880f992. The modifies_file() gate is sound: modifies_file already exists on the VersionFile trait with a true default and only gomod overrides it, and formats/mod.rs has tests pinning both sides of that, so the gate cannot silently widen. One nit on the test it ships with.
|
Right, and it is the exact failure mode I have been trying to avoid elsewhere in this PR. Applied: it now asserts Both guards are needed and neither is redundant: the |
Closes #983.
compute_planread the package's first versioned file withread_version(...).ok(), so a path that does not exist becameNoneand planning carried on.checkthen reported a clean plan, exit 0, for a configuration that cannot be released. The only signal was the missingover <file>suffix on that package's line, which is exactly what nobody notices in a list of packages.Correcting the issue's stated impact
The issue says the release "runs, tags are created and the changelog is written, but the version file is never bumped". That is not what happens, at least not for a format that writes the file. Running a real
releaseon the issue's own repro againstmain:write_versionreads the file before editing it, so it fails and no tag is created. There is no silent drift and nothing to clean up afterwards.What is real is narrower and still worth fixing:
checkdisagrees withrelease. A dry run whose whole job is to say what a release will do reports success for somethingreleaserefuses, and when it does refuse, the message is a bare read error with no hint about the path convention. This PR makescheckanswer the question it was asked, at the point where the answer is useful.The error message and the E1024 docs are worded to that, rather than to the tag-drift claim.
Two scoping choices
Only packages this run would write. The check sits in
compute_planafter the excluded and not-touched early returns, rather than running across every configured package the wayvalidatedoes. A repository-wide check turns a partial or sparse checkout into a hard failure: a job that checks out only the packages it touches would stop being able to release any of them.validateanddoctorstill cover every configured package, which is their job.Only formats that write the file.
gomodis the case that matters: a Go module's version lives in the git tag,read_versiongets it fromgit describe, andwrite_versionis a no-op. A missinggo.modcannot cause the failure this check exists to catch, and theformat-gomodfixture is a real repository that has nogo.modat all. Caught by CI, which is exactly what that fixture is for.Tests
Four, on real git fixtures:
a_versioned_file_that_does_not_exist_fails_the_plan_rather_than_bumping_nothingreproduces the issue's config verbatim and asserts the error namesapi/Cargo.toml.a_versioned_file_that_exists_still_plans_normallyis the control, so the check cannot pass by rejecting everything.an_untouched_package_is_skipped_before_its_files_are_checkedpins the first scoping choice.a_format_that_never_writes_the_file_does_not_need_it_to_existpins the second, and asserts the fixture really has nogo.modfirst so it cannot pass vacuously.Known red check:
multi-versioned-filesFixtures — Config & Formatsis still failing on that one fixture, and the fixture is wrong rather than the change.multi-versioned-files.jsondeclares two toml versioned files,version.tomlandversion2.toml. The generator only ever writes oneversion.tomlper package, soversion2.tomlhas never existed. The fixture asserts thatcheckplans this repository fine, whileferrflow releaseon it would fail atversion2.tomlexactly as above. This PR makescheckagree withrelease, so the fixture's expectation no longer holds.It cannot be fixed from this repo.
PackageDefin the generator has noversioned_filesfield, so the block thatformat-gomod.jsonalready carries is silently dropped by serde. Making the fixture honest means:versioned_filestoPackageDefand materialising them,ci.yml(currently7b04ba1/ v1.2.2) here.Same shape as the schema-parity dance, and this PR stays red until step 3. Say the word and I will open the Fixtures side. The alternative, dropping the second file from the fixture, would delete the only coverage of multiple versioned files, which seems worse.