Relax container image schema and add custom validation#323
Open
ericsciple wants to merge 2 commits intomainfrom
Open
Relax container image schema and add custom validation#323ericsciple wants to merge 2 commits intomainfrom
ericsciple wants to merge 2 commits intomainfrom
Conversation
ba800d5 to
17c2300
Compare
Related PR: - actions/runner#4220 Relaxing schema non-empty-string for container/service image and moving to custom validation. This matches current production behavior which allows empty string at runtime, but not parse time.
17c2300 to
671f92d
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request relaxes the JSON schema validation for container and service image properties and adds custom validation logic to match production runtime behavior. The schema type is changed from "non-empty-string" to "string", allowing empty strings to pass schema validation, while custom validation in the converter layer enforces the appropriate rules.
Changes:
- Schema relaxation: Changed container/service image type from
"non-empty-string"to"string"in workflow-v1.0.json - Custom validation: Implemented detailed validation logic in container.ts that handles empty strings,
docker://prefixes, expressions, and the distinction between job containers (silent on empty) and service containers (error on empty) - Error handling: Added
handleTemplateTokenErrorswrapper to container and services conversion calls in job.ts - Comprehensive test coverage: Added 31 test cases covering shorthand/mapping forms, expression safety, and all error scenarios
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| workflow-parser/src/workflow-v1.0.json | Changed container and service image schema type from "non-empty-string" to "string" to allow empty strings at parse time |
| workflow-parser/src/model/converter/container.ts | Implemented custom validation for container images including empty string detection, docker:// prefix handling, expression key/value handling, and isServiceContainer flag support |
| workflow-parser/src/model/converter/job.ts | Wrapped convertToJobContainer and convertToJobServices calls with handleTemplateTokenErrors for proper error handling |
| workflow-parser/src/model/converter/container.test.ts | Added comprehensive test suite with 31 test cases covering all validation scenarios and expression safety |
| workflow-parser/testdata/skipped-tests.txt | Added job-container-invalid.yml to skipped tests list |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Add containerImageValidation experimental feature flag that gates the new container image validation behind an opt-in toggle. When the flag is off (default), the legacy converter logic is used. When enabled, the improved validation with expression handling runs. The legacy code is duplicated to keep code paths fully isolated and make the eventual cleanup diff minimal — just delete the legacy functions and the flag guards. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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
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.
Related PR:
Relaxing schema
non-empty-stringfor container/service image and moving to custom validation. This matches current production behavior which allows empty string at runtime, but not parse time.Parse Time Behavior
container: ''container: 'docker://'container: {}container: {image: ''}container: {image: 'docker://'}container: {image: '', options: ${{ expr }}}services: {svc: ''}services: {svc: 'docker://'}services: {svc: {}}services: {svc: {image: ''}}