Repository navigation
fix: warn on a symlinked top-level architecture directory instead of dropping it silently - #1290
Merged
Merged
Conversation
…dropping it silently Both scripts that enumerate content/en/architectures/ in the cloned cncf/architecture repository filtered on Dirent.isDirectory(), which is false for a symlink. A symlinked architecture directory upstream was therefore excluded with no log output: - import-architectures.mjs prunes every imported page before the walk, so the architecture's page was deleted and it vanished from the site and from catalog.json with nothing in the import log (#1267). - collect-metrics.mjs published an undercounted Reference architectures metric that validate-metrics.mjs never checks (#1276). Guard the top-level filter the way walkFiles() already guards every other symlink in the clone: test isSymbolicLink() first and console.warn each skipped entry so the drop is visible in the workflow log. The two scripts now agree on which directories exist. runScriptInSandbox gains a repoSymlinks option mirroring the import harness's upstreamSymlinks, so a fixture clone can carry a symlink the same way a committed one would. Closes #1267 Closes #1276 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Bob Killen <bkillen@linuxfoundation.org>
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.
Closes #1267. Closes #1276.
One PR for both, as proposed in #1283: the two findings are the same unguarded
Dirent.isDirectory()filter over the same third-party input, at two sites.Finding
Both scripts that enumerate
content/en/architectures/in the clonedcncf/architecturerepository filtered onentry.isDirectory(), which isfalsefor a symlink. A symlinked architecture directory upstream was therefore excluded with no log output:scripts/import-architectures.mjsprunes every imported page before the walk, so the architecture's page was deleted and it vanished from the site and fromcatalog.jsonwith nothing in the import log ([scanner] import-architectures.mjs silently drops a symlinked top-level architecture directory from the upstream clone #1267).scripts/collect-metrics.mjspublished an undercounted "Reference architectures" metric thatvalidate-metrics.mjsnever checks ([scanner] collect-metrics.mjs silently drops symlinked architecture dirs from the Reference architectures count #1276).Change
Guard the top-level filter the way
walkFiles()already guards every other symlink in the clone (#533, #548): testisSymbolicLink()first andconsole.warneach skipped entry so the drop is visible in the workflow log. Skipped rather than resolved throughrealpathSync, so the two scripts agree on which directories exist and the count matches the import.tests/helpers-script-sandbox.mjsgains arepoSymlinksoption mirroring the import harness'supstreamSymlinks; thegit cloneshim already copies fixtures withcp -R, which preserves links.Tests
tests/import-architectures.test.mjs: a link to a directory outside the tree and a link aliasing a sibling architecture are both skipped and warned about; the real sibling is imported once; neither link produces a page.tests/collect-metrics.test.mjs: a link to a sibling and a dangling link are both skipped and warned about; the count stays at the two real directories.npm run test:unit:coverage:check: 2317 tests pass, exit 0; both scripts remain at 100% lines and regions.check:formatandcheck:spellingclean.