Conversation
There was a problem hiding this comment.
Pull request overview
Replaces the Python-based PR-check workflow generator with a TypeScript implementation, updating the workflow-generation toolchain and regenerating the affected __*.yml workflows under .github/workflows.
Changes:
- Added
pr-checks/sync.ts(TypeScript replacement forsync.py) using theyamlpackage to preserve step formatting/comments. - Updated
pr-checks/sync.shto run the new generator viatsxand introduced a dedicatedpr-checks/package.json+ lockfile. - Regenerated the
__*.ymlworkflows and made small formatting adjustments in a couple of check specs.
Reviewed changes
Copilot reviewed 42 out of 45 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tsconfig.json | Excludes pr-checks from the main TypeScript compilation scope. |
| eslint.config.mjs | Ignores pr-checks/** for ESLint runs. |
| pr-checks/sync.ts | New TypeScript workflow generator (replaces Python behavior). |
| pr-checks/sync.sh | Switches generator execution from Python/ruamel to Node/tsx. |
| pr-checks/package.json | Adds Node dependencies needed to run sync.ts. |
| pr-checks/package-lock.json | Locks pr-checks generator dependencies for reproducibility. |
| pr-checks/.gitignore | Ignores pr-checks/node_modules. |
| pr-checks/checks/upload-sarif.yml | Minor formatting/quoting changes in if: conditions. |
| pr-checks/checks/bundle-from-nightly.yml | Minor formatting change to if: condition style. |
| .github/workflows/__all-platform-bundle.yml | Regenerated workflow output (formatting/quoting changes). |
| .github/workflows/__analysis-kinds.yml | Regenerated workflow output (formatting/quoting changes). |
| .github/workflows/__analyze-ref-input.yml | Regenerated workflow output (formatting/quoting changes). |
| .github/workflows/__autobuild-action.yml | Regenerated workflow output (formatting/quoting changes). |
| .github/workflows/__autobuild-direct-tracing-with-working-dir.yml | Regenerated workflow output (formatting/quoting changes). |
| .github/workflows/__build-mode-autobuild.yml | Regenerated workflow output (formatting/quoting changes). |
| .github/workflows/__build-mode-manual.yml | Regenerated workflow output (formatting/quoting changes). |
| .github/workflows/__build-mode-none.yml | Regenerated workflow output (formatting/quoting changes). |
| .github/workflows/__build-mode-rollback.yml | Regenerated workflow output (formatting/quoting changes). |
| .github/workflows/__bundle-from-nightly.yml | Regenerated workflow output (formatting/quoting changes). |
| .github/workflows/__cleanup-db-cluster-dir.yml | Regenerated workflow output (formatting/quoting changes). |
| .github/workflows/__config-export.yml | Regenerated workflow output (formatting/quoting changes). |
| .github/workflows/__diagnostics-export.yml | Regenerated workflow output (formatting/quoting changes). |
| .github/workflows/__export-file-baseline-information.yml | Regenerated workflow output (formatting/quoting changes). |
| .github/workflows/__go-custom-queries.yml | Regenerated workflow output (formatting/quoting changes). |
| .github/workflows/__go-indirect-tracing-workaround-diagnostic.yml | Regenerated workflow output (formatting/quoting changes). |
| .github/workflows/__go-indirect-tracing-workaround-no-file-program.yml | Regenerated workflow output (formatting/quoting changes). |
| .github/workflows/__init-with-registries.yml | Regenerated workflow output (formatting/quoting changes). |
| .github/workflows/__job-run-uuid-sarif.yml | Regenerated workflow output (formatting/quoting changes). |
| .github/workflows/__language-aliases.yml | Regenerated workflow output (formatting/quoting changes). |
| .github/workflows/__local-bundle.yml | Regenerated workflow output (formatting/quoting changes). |
| .github/workflows/__multi-language-autodetect.yml | Regenerated workflow output (formatting/quoting changes). |
| .github/workflows/__packaging-codescanning-config-inputs-js.yml | Regenerated workflow output (formatting/quoting changes). |
| .github/workflows/__packaging-config-inputs-js.yml | Regenerated workflow output (formatting/quoting changes). |
| .github/workflows/__packaging-config-js.yml | Regenerated workflow output (formatting/quoting changes). |
| .github/workflows/__packaging-inputs-js.yml | Regenerated workflow output (formatting/quoting changes). |
| .github/workflows/__remote-config.yml | Regenerated workflow output (formatting/quoting changes). |
| .github/workflows/__resolve-environment-action.yml | Regenerated workflow output (formatting/quoting changes). |
| .github/workflows/__split-workflow.yml | Regenerated workflow output (formatting/quoting changes). |
| .github/workflows/__start-proxy.yml | Regenerated workflow output (formatting/quoting changes). |
| .github/workflows/__submit-sarif-failure.yml | Regenerated workflow output (formatting/quoting changes). |
| .github/workflows/__swift-custom-build.yml | Regenerated workflow output (formatting/quoting changes). |
| .github/workflows/__unset-environment.yml | Regenerated workflow output (formatting/quoting changes). |
| .github/workflows/__upload-ref-sha-input.yml | Regenerated workflow output (formatting/quoting changes). |
| .github/workflows/__upload-sarif.yml | Regenerated workflow output (formatting/quoting changes). |
| .github/workflows/__with-checkout-path.yml | Regenerated workflow output (formatting/quoting changes). |
Files not reviewed (1)
- pr-checks/package-lock.json: Language not supported
esbena
left a comment
There was a problem hiding this comment.
(submitting probably empty review for technical reasons, complete review will follow)
| "setup-kotlin": String( | ||
| !("container" in checkSpecification), |
There was a problem hiding this comment.
This seems somehow like the dual of the (convoluted IMO) isTruthy introduced earlier.
My preference is to be more explicit here with a ternary that produces literal true/false:
| "setup-kotlin": String( | |
| !("container" in checkSpecification), | |
| "setup-kotlin": "container" in checkSpecification? "false": "true" |
| extraGroupName += "-${{inputs." + inputName + "}}"; | ||
| } |
There was a problem hiding this comment.
Lets do a very simple sanitization check here, namely that the input name is a valid identifier.
It's not so much for security I worry, it's rather just the ease of diagnosing invalid workflow files.
| combinedInputs = { ...combinedInputs, ...checkInputs }; | ||
|
|
||
| for (const inputName of Object.keys(checkInputs)) { | ||
| checkWith[inputName] = "${{ inputs." + inputName + " }}"; |
There was a problem hiding this comment.
Ditto the earlier comment about sanitization
| let useAllPlatformBundle = "false"; // Default to false | ||
| if (checkSpecification.useAllPlatformBundle) { | ||
| useAllPlatformBundle = checkSpecification.useAllPlatformBundle; | ||
| } |
There was a problem hiding this comment.
This should be a one liner.
| let useAllPlatformBundle = "false"; // Default to false | |
| if (checkSpecification.useAllPlatformBundle) { | |
| useAllPlatformBundle = checkSpecification.useAllPlatformBundle; | |
| } | |
| const useAllPlatformBundle = checkSpecification.useAllPlatformBundle? checkSpecification.useAllPlatformBundle: false; |
or
| let useAllPlatformBundle = "false"; // Default to false | |
| if (checkSpecification.useAllPlatformBundle) { | |
| useAllPlatformBundle = checkSpecification.useAllPlatformBundle; | |
| } | |
| const useAllPlatformBundle = !!checkSpecification.useAllPlatformBundle; |
| fs.writeFileSync(filePath, stripTrailingWhitespace(header + yamlStr), "utf8"); | ||
| } | ||
|
|
||
| function isTruthy(value: string | boolean | undefined): boolean { |
There was a problem hiding this comment.
This function needs a comment, but I'm not sure about the existence at all actually. The truthiness check in JavaScript is widely known to be !!value. This implementation changes it very subtly.
Perhaps a rename to something like isBooleanTrueOrStringTrue?
|
LGTM, I only have some stylistic comments. The migration is pleasantly simple, and the near zero-diff strategy seems robust enough here. |
This PR takes a first step at converting a script from Python to TypeScript. Concretely, it replaces the
sync.pyscript that turns our PR check specifications into GitHub Actions workflows with a roughly equivalent TypeScript replacement.Initially, I aimed for exact textual equivalence in the output between the two scripts, but then I ultimately allowed a few minor changes:
stepsfrom the specification is now preserved more accurately.stepsare converted to single quoted strings.yamlpackage allows for unlimited line length, whichruamel.yamldidn't quite handle correctly before, so some lines are no longer wrapped as intended.I initially started by using
js-yaml, since we already use this elsewhere in the repo, but then switched toyamlsince it gave me more control over the formatting.Follow-up in #3529 for
sync_back.py.Risk assessment
For internal use only. Please select the risk level of this change:
Which use cases does this change impact?
Environments:
How did/will you validate this change?
pr-checks).If something goes wrong after this change is released, what are the mitigation and rollback strategies?
How will you know if something goes wrong after this change is released?
Are there any special considerations for merging or releasing this change?
Merge / deployment checklist