fix(ci-compliance): exclude versioning upgrade maps in the runner, not per repo - #23
Merged
sakanni merged 1 commit intoSep 29, 2026
Conversation
…t per repo ci-dataset-compliance selects files by a substring test over the whole path, then asserts each one deserialises into a BH.oM.Data.Library.Dataset. Versioning upgrade maps, named Versioning_<digits>.json, are never Dataset documents, so any that the selector reaches fails. The selector reaches them when a directory supplies the "datasets" substring, which in practice is a project directory named *_Datasets. Measured: BuroHappold_Datasets#163 carries one such file and it is the single failure annotation on that check, against six real datasets read correctly. The exclusion was being added to each repository's pathspec instead. That is a general rule about a file type, so review asked for it centrally, and a per-repository pathspec cannot be the place: a repository that never receives the edit stays wrong, and nothing propagates a template change to an onboarded repository. IsDatasetFile is the layer that is correct however the file was selected, whether from a pathspec, a refreshed template, or a direct invocation. It is also the only one that arrives without a distribution wave: prepare-runner keys its cache on a content hash of tools/ComplianceRunner/src when the action is referenced by branch, which every consumer does, so the change lands on the first run after merge. The rule is anchored on the file name and gated on a digit, so Versioning_Rules.json stays in scope and a directory named Versioning_93 does not take its contents out of scope. Discriminating on content instead, by requiring a top-level _t, was rejected: it would also skip a real Dataset that had lost its _t, which is a defect this check exists to catch. This diverges from BHoMBot's DatasetCompliance, which applies the bare path test and still holds the required context. The docstring no longer claims they agree. 88 unit tests and 66 integration tests pass, 17 and 2 of them new. Five mutation checks ran against the new rule, covering the exclusion itself, the digit gate, the file-name anchor, the separator handling and the prefix anchor. Each failed tests and reverted clean.
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.
Issues addressed by this PR
ci-dataset-complianceasserts that every file it selects deserialises into aBH.oM.Data.Library.Dataset. Its selector is a substring match over the whole path, so it also reachesVersioning_<digits>.jsonupgrade maps, which hold{"Dataset":{"ToNew":..,"ToOld":..}}, carry no_t, and can never be Dataset documents. The runner now declines them, exactly as it already declines a path with nodatasetssubstring.The exposure is narrower than it first appears. The selector needs
datasetssomewhere in the path, so an upgrade map in an ordinary oM directory is already out of scope. What reaches it is a repository whose project directory is itself named for datasets, such asBHoM_Datasets, where that directory supplies the substring and the file name is the only remaining signal. No such file is selected on any default branch today; one is on an open pull request, where it is the single failing file against six datasets read correctly.This was being added to each repository's dataset pathspec instead, and review asked for it centrally. That is right: it describes a file type any versioning repository can hold rather than anything about one repository. A pathspec could not be the place in any case, because a repository that never receives the edit stays wrong and nothing propagates a workflow change to an already-onboarded repository.
FileFilteris correct however the file was selected, and needs no distribution wave:prepare-runnerkeys its cache on a content hash oftools/ComplianceRunner/srcwhenever the action is referenced by branch.Test files
FileFilterTestsandDatasetComplianceRunnerE2ETests, 17 and 2 new assertions, 88 and 66 passing with existing tests unmodified. Five mutation checks ran against the new rule and each failed tests and reverted clean.Changelog
ci-dataset-complianceno longer reports onVersioning_<digits>.jsonversioning upgrade maps.Additional comments
The rule is anchored on the file name and gated on a digit, so
Versioning_Rules.jsonstays in scope and a directory namedVersioning_93does not take its contents out of scope.Discriminating on content instead, by requiring a top-level
_t, was considered and rejected. It reaches the same files, but it would also skip a real Dataset that had lost its_t, turning a defect this check exists to catch into a silent pass. A file-name rule cannot do that, because the name is generated by the versioning pipeline and never belongs to a Dataset.This diverges from BHoMBot's
DatasetCompliance, which applies the bare path test and still holds the required context where this fires. The two now disagree by design, and the docstring no longer claims they match.