Skip to content

Add support for validation jobs to sync.ts, and refactor#3536

Closed
mbg wants to merge 5 commits intomainfrom
mbg/pr-checks/validation-jobs
Closed

Add support for validation jobs to sync.ts, and refactor#3536
mbg wants to merge 5 commits intomainfrom
mbg/pr-checks/validation-jobs

Conversation

@mbg
Copy link
Member

@mbg mbg commented Mar 3, 2026

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 main function that we had initially ported from the earlier sync.py script 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:

  • Low risk: Changes are fully under feature flags, or have been fully tested and validated in pre-production environments and are highly observable, or are documentation or test only.

Which use cases does this change impact?

Environments:

  • Testing/None - This change does not impact any CodeQL workflows in production.

How did/will you validate this change?

  • End-to-end tests - I am depending on PR checks (i.e. tests in pr-checks).

If something goes wrong after this change is released, what are the mitigation and rollback strategies?

  • Development/testing only - This change cannot cause any failures in production.

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?

  • No special considerations - This change can be merged at any time.

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Consider adding a changelog entry for this change.
  • Confirm the readme and docs have been updated if necessary.

@mbg mbg self-assigned this Mar 3, 2026
Copilot AI review requested due to automatic review settings March 3, 2026 15:01
@mbg mbg requested a review from a team as a code owner March 3, 2026 15:01
@github-actions github-actions bot added the size/L May be hard to review label Mar 3, 2026
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 into generateJobMatrix(), getSetupSteps(), generateJob(), generateValidationJob(), and generateValidationJobs(), and extracts language setup configuration into defaultLanguageVersions and languageSetups constants.
  • New feature: adds a validationJobs property to the Specification interface and a new JobSpecification interface 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;
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
specProperty: keyof Specification;
specProperty: keyof JobSpecification;

Copilot uses AI. Check for mistakes.
Comment on lines 642 to +644
jobs: {
[checkName]: checkJob,
...validationJobs,
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Comment on lines +70 to +76
installNode?: boolean;
installGo?: boolean;
installJava?: boolean;
installPython?: boolean;
installDotNet?: boolean;
installYq?: boolean;
}
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
@mbg mbg closed this Mar 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/L May be hard to review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants