Add support for validation jobs to sync.ts, and refactor#3536
Add support for validation jobs to sync.ts, and refactor#3536
sync.ts, and refactor#3536Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for optional validationJobs in the pr-checks/sync.ts synchronization script, which generates GitHub Actions workflow files from PR check specification YAML files. The motivation is to allow a second job to run after the main check job (e.g., to verify that an artifact exists). The PR also refactors the previously monolithic main() function into smaller, focused helper functions and extracts language/framework setup configuration into a centralized languageSetups constant, resulting in alphabetically-ordered setup steps in the generated workflows.
Changes:
- Refactoring: splits the
main()function intogenerateJobMatrix(),getSetupSteps(),generateJob(),generateValidationJob(), andgenerateValidationJobs(), and extracts language setup configuration intodefaultLanguageVersionsandlanguageSetupsconstants. - New feature: adds a
validationJobsproperty to theSpecificationinterface and a newJobSpecificationinterface so that post-check validation jobs can be defined in spec files. - Generated workflow files are regenerated with alphabetical ordering of language setup steps and workflow input definitions.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
pr-checks/sync.ts |
Major refactor: new interfaces, functions, and validationJobs support |
.github/workflows/__*.yml |
Auto-generated; step ordering changed to alphabetical (csharp → go → node → python); no behavioral impact |
|
|
||
| /** Describes language/framework-specific steps and inputs. */ | ||
| interface LanguageSetup { | ||
| specProperty: keyof Specification; |
There was a problem hiding this comment.
The specProperty field in the LanguageSetup interface is typed as keyof Specification, but since the install flag properties (installGo, installNode, etc.) are defined in JobSpecification, a more precise and accurate type would be keyof JobSpecification. The current keyof Specification type is overly broad — it permits any key from the full Specification interface (such as "versions", "collection", or "container") to be used as a specProperty, which would be semantically incorrect. Using keyof JobSpecification would constrain this to only properties that are meaningful install flags.
| specProperty: keyof Specification; | |
| specProperty: keyof JobSpecification; |
| jobs: { | ||
| [checkName]: checkJob, | ||
| ...validationJobs, |
There was a problem hiding this comment.
If a key in validationJobs matches checkName, the spread ...validationJobs will silently overwrite the main job in the jobs object, resulting in a broken workflow with no main check job. There is no validation to detect and reject this collision. Consider adding a guard to throw an error if any validation job key equals checkName.
| installNode?: boolean; | ||
| installGo?: boolean; | ||
| installJava?: boolean; | ||
| installPython?: boolean; | ||
| installDotNet?: boolean; | ||
| installYq?: boolean; | ||
| } |
There was a problem hiding this comment.
The JobSpecification interface includes installNode, installGo, installJava, installPython, installDotNet, and installYq properties, which are also applicable to validation jobs (since they are typed as JobSpecification). However, generateValidationJob does not process any of these properties, so if a validation job spec sets any of them, they will be silently ignored and the corresponding setup steps won't be added. This could confuse users who try to use them in a validation job specification. Consider either removing these properties from JobSpecification (moving them back to Specification only), or implementing support for them in generateValidationJob.
The primary goal of this PR is to add support for additional, validation jobs to
sync.ts. My motivation for this is #3519 (review). To add a PR check which does that, the most sensible way of testing it would be to run a second job after the main one which verifies that the artifact exists.I have refactored a few things in the first commits to break up the horrible long
mainfunction that we had initially ported from the earliersync.pyscript as-is in #3526.The refactoring has introduced some minor changes to the generated workflow files because the order of the various
setup-*steps is now alphabetical, rather than effectively random. This should not have any impact on the behaviour of the workflows.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?
N/A
Are there any special considerations for merging or releasing this change?
Merge / deployment checklist