pipelines: opt-in placeholder mode for schema validation#237
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces an opt-in “placeholder mode” for pipeline schema validation, allowing placeholder strings (regex-matched) to be accepted where the schema expects non-string scalars. This is aimed at validating templated pipeline.yaml rendered against a fully “dunderized” config while preserving strict validation as the default for all existing callers.
Changes:
- Added
ValidateOptionplumbing andWithAllowPlaceholders(pattern)(default^__.+__$when pattern is empty). - Added schema “widening” logic that rewrites decoded JSON Schema in-memory to allow placeholder strings for non-string scalars before compiling.
- Added a new
ValidatePipelineSchemaWithOptionsentry point and threaded options throughNewPipelineFromFile/NewPipelineFromBytes, plus new tests for placeholder behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
pipelines/types/options.go |
Adds validation option types and WithAllowPlaceholders defaulting behavior. |
pipelines/types/widen.go |
Implements in-memory schema traversal and widening to accept placeholder strings for certain scalar schema nodes. |
pipelines/types/util.go |
Splits schema decode/compile, adds ValidatePipelineSchemaWithOptions, and applies widening when placeholder mode is enabled. |
pipelines/types/pipeline.go |
Threads validation options through pipeline constructors and uses the new validation entry point. |
pipelines/types/util_test.go |
Adds test coverage for strict vs placeholder validation scenarios and backwards-compat behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Split compileSchema(ref, bytes) into decodeSchemaMap and compileSchemaFromMap so future callers can rewrite the schema between decode and compile. Behavior of compileSchema is unchanged; it now wraps the two halves. Refs: AROSLSRE-900
Add a WithAllowPlaceholders ValidateOption that, when supplied to the new ValidatePipelineSchemaWithOptions (or to NewPipelineFromFile / NewPipelineFromBytes), widens the pipeline schema in-memory so that every non-string scalar field (boolean / integer / number, and pure non-string enums) also accepts a string matching a placeholder pattern. The default pattern is ^__.+__$, matching sdp-pipelines dunder configuration output. The schema asset on disk is unchanged. ValidatePipelineSchema retains strict semantics; only callers that opt in are affected. The widener resolves local \ targets and recurses through standard JSON Schema subtree keywords (properties, items, oneOf, allOf, etc.) with a visited-set guard against cycles. This unblocks sdp-pipelines dunder validation of ARO-HCP pipelines where templated boolean fields (e.g. skipSync, useBetaEndpoint) would otherwise need conditional rendering workarounds. Refs: AROSLSRE-900 Refs: Azure/ARO-HCP#5339
Covers strict-mode regression guards (placeholders rejected by default), placeholder-mode acceptance for boolean / integer / $ref-resolved scalars, pattern guard for non-matching strings, custom-pattern honoring, and a backwards-compatibility assertion that the legacy ValidatePipelineSchema entry point remains strict. Refs: AROSLSRE-900
- shouldWidenNode now widens nodes that declare a non-string `const`
(mirroring the existing non-string `enum` branch). Without this, a
templated placeholder for a field constrained by `const: <non-string>`
would still be rejected even with WithAllowPlaceholders. The current
schema only contains one such site (publicSource: {const: true} on
imageMirrorStep), but the principle generalises and is now pinned by
direct widener tests.
- resolveLocalRef switches to strconv.Atoi for parsing array-index
pointer segments, dropping the custom atoi reimplementation and
associated error type. This eliminates the int-overflow edge case
flagged in review.
- pipelines/types/widen_test.go pins the structural rewrite invariants
per node shape (type/enum/const, with and without a paired type
keyword) and across a $ref-resolved const target so future refactors
of the widener cannot silently drop a branch.
Refs: AROSLSRE-900
4873d04 to
ea03277
Compare
Move PlaceholderGenerator, NewDunderPlaceholders, and the EV2Mapping walker from sdp-pipelines/tooling/pkg/ev2/manifests/generate into pipelines/types so the placeholder producer and the schema validator that consumes it share a home and a single regex constant (DefaultPlaceholderPattern). Arrays in per-region configuration are documented as unsupported (see ARO-HCP docs/configuration.md#limitations), so the moved walker treats slices as opaque scalars rather than walking element-by-element. The matching sdp-pipelines slice walker and Azure#239 (slice indexing on GetByPath) are being closed for the same reason. Adds an end-to-end test demonstrating the producer + validator contract: EV2Mapping with NewDunderPlaceholders produces a Configuration whose leaves match DefaultPlaceholderPattern; the resulting dunder-rendered pipeline.yaml template is accepted by ValidatePipelineSchemaWithOptions in placeholder mode and rejected by strict mode (regression guard). Rendering the same template with the real typed configuration is accepted by strict mode in both the lower-level validator and the full NewPipelineFromBytes flow, confirming the move-in does not change non-placeholder behavior.
|
Update pushed and PR body reframed around the scalar-only path:
The follow-up is now smaller: after this merges and an ARO-Tools tag is cut, sdp-pipelines can import the dunder producer from ARO-Tools and opt into |
Problem
sdp-pipelinesvalidatespipeline.yamlafter rendering it with a dunder config. That render turns every scalar config value into a string placeholder, which breaks schema validation when the pipeline field is a boolean, integer, or number.Goal
Make the dunder validation pass understand scalar placeholders without weakening normal pipeline validation. Strict validation remains the default for real typed config and all existing callers.
This PR is intentionally scalar-only. Dynamic
{{ if }}/{{ range }}pipeline generation and per-region lists are out of scope because Ev2 manifests are generated up front and ARO-HCP config should avoid arrays: https://github.com/Azure/ARO-HCP/blob/main/docs/configuration.md#limitations.What changes
Adds opt-in placeholder validation with
WithAllowPlaceholders(pattern)andValidatePipelineSchemaWithOptions(...). When enabled, the schema is widened in memory so non-string scalar nodes (boolean,integer,number, and non-string enums, including$reftargets) also accept placeholder strings matching the configured pattern.No schema file is changed on disk.
ValidatePipelineSchema(...),NewPipelineFromFile(...), andNewPipelineFromBytes(...)keep strict behavior unless a caller explicitly passes the new option.Dunder producer move-in
Moves the scalar dunder placeholder producer from
sdp-pipelinesintopipelines/types. This gives the producer and validator one shared home and one shared default placeholder contract:DefaultPlaceholderPattern = ^__.+__$.The moved API is
PlaceholderGenerator,NewDunderPlaceholders(), andEV2Mapping(...). Slices are treated as opaque scalar leaves rather than walked element-by-element, matching the array/list policy above.Tests
The tests cover strict vs placeholder validation for booleans, integers,
$refscalar targets, custom placeholder patterns, and backwards compatibility. They also cover the moved dunder producer and its regex contract with the validator.The end-to-end test shows the intended flow:
Follow-ups
After this merges and a new ARO-Tools tag includes it,
sdp-pipelinescan import the dunder producer from ARO-Tools and passtypes.WithAllowPlaceholders("")at the dunder validation site. After that deploys, ARO-HCP can revert theskipSync{{ if }}workaround from Azure/ARO-HCP#5339.Closed as part of this pivot: #239, sdp-pipelines ADO PR 15824468, and AROSLSRE-914. Those tracked the slice/list path, which is no longer policy-compliant.