Skip to content

config/types: support slice indexing in GetByPath#239

Closed
raelga wants to merge 1 commit into
Azure:mainfrom
raelga:config-getbypath-slice-indexing
Closed

config/types: support slice indexing in GetByPath#239
raelga wants to merge 1 commit into
Azure:mainfrom
raelga:config-getbypath-slice-indexing

Conversation

@raelga
Copy link
Copy Markdown
Contributor

@raelga raelga commented May 21, 2026

Summary

Teach Configuration.GetByPath to navigate slice ([]any) nodes by interpreting the next dot-segment as a non-negative integer index. Map navigation is unchanged.

After this change, a config reference like imageRegistryPolicy.extraAllowedRegistries.0 resolves to the first element of that slice, matching the existing dot-path conventions for map-valued configuration.

Why

Configuration values can be slices today, but GetByPath could only return a whole slice as an opaque value — there was no way to address an individual element. That blocks downstream tooling (notably the sdp-pipelines dunder rendering pipeline) from producing per-index placeholder references for list-valued configuration during ARO-HCP Ev2 manifest generation.

A list-valued field that uses Go + "{{ range }}" + in a Helm values.yaml currently has no working dunder representation: the build-time mapping collapses the entire slice to a single scalar placeholder, and + "text/template" + then fails with + "range can't iterate over " + (e.g. Azure/ARO-HCP#4690 hitting __imageRegistryPolicy.extraAllowedRegistries__).

This PR is the first of two changes that, together, fix that — it unblocks a sdp-pipelines change that emits per-index dunder placeholders (__path.0__, __path.1__, ...) and resolves them through GetByPath. That second PR will land separately in Azure/sdp-pipelines once this is merged and tagged.

Behaviour

Path Before After
items (slice node) returns whole []any returns whole []any (unchanged)
items.0 error returns element 0
items.5 (len 3) error *IndexOutOfRangeError
items.foo error error: "expected non-negative integer..."
nested.key.deep (deep map walk) unchanged unchanged

A new IndexOutOfRangeError sentinel lets callers distinguish out-of-bounds from other failures without string matching, in the same shape as the existing MissingKeyError. The fall-through error wording changes from "expected nested map" to "expected nested map or slice" to accurately describe the new capability — one existing test that exact-matched that string was updated.

Tests

  • config/types.TestGetByPathSlices (new, 13 cases): top-level slice index, last element, slice-of-map, slice-of-slice, the realistic image-registry-policy shape, and three error paths (missing key, out-of-range, non-numeric slice key).
  • config.TestGetByPath (existing, 4 cases): error-string expectation updated; otherwise unchanged.
  • go vet ./... clean.

Related

Until now, Configuration.GetByPath only walked map[string]any nodes; a
slice value was an opaque leaf that could only be retrieved as a whole.
This made it impossible to author a config reference of the form
`imageRegistryPolicy.extraAllowedRegistries.0` to address a single
element of a list-valued configuration, even though list values are
otherwise first-class in our configuration shape (and the sdp-pipelines
dunder rendering pipeline needs them to be).

This change teaches GetByPath that a key applied to a []any node is a
non-negative integer index into that slice. Existing behaviour for
map navigation is unchanged. The error wording in the fall-through is
adjusted ("nested map or slice") to reflect the new capability.

Errors:
- MissingKeyError continues to be returned for missing map keys.
- A new IndexOutOfRangeError is returned for negative or out-of-bounds
  indices so callers can distinguish that failure mode without string
  matching.
- A non-numeric key applied to a slice returns a plain wrapped error.

Tests cover top-level slice indexing, deep navigation through
slice-of-map and slice-of-slice, the realistic image-registry-policy
shape, and the three error paths (missing key, out-of-range index,
non-numeric slice key). The existing TestGetByPath error-string
expectation is updated to match the new wording.

Refs: AROSLSRE-913
Copilot AI review requested due to automatic review settings May 21, 2026 15:32
Copy link
Copy Markdown

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

Adds slice ([]any) traversal support to config/types.Configuration.GetByPath, allowing dot-path segments to index into list-valued configuration (e.g., items.0) while preserving existing map traversal behavior. This unblocks downstream tooling that needs per-index config references when rendering list-valued template placeholders.

Changes:

  • Extend Configuration.GetByPath to handle slice nodes by parsing the next dot-segment as an integer index.
  • Introduce IndexOutOfRangeError to distinguish out-of-bounds slice indexing from other failures.
  • Add dedicated slice-navigation test coverage and update an existing error-string expectation.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
config/types/configuration.go Implements slice indexing in GetByPath and adds IndexOutOfRangeError.
config/types/configuration_test.go Adds a new table-driven test suite covering slice navigation and error cases.
config/types_test.go Updates an expected error string to reflect the new “map or slice” wording.
Comments suppressed due to low confidence (1)

config/types/configuration.go:76

  • The slice-index parsing uses strconv.Atoi, which accepts leading signs. That means a path segment like "-0" parses to 0 and will successfully index the slice, even though the docstring/error message says the key must be a non-negative integer literal. Consider validating the segment first (e.g., require ^[0-9]+$ and then parse), and treat negative indices consistently (either reject them with the "expected non-negative integer" error or explicitly document/support them).
		case []any:
			idx, err := strconv.Atoi(key)
			if err != nil {
				return nil, fmt.Errorf("configuration%s: expected non-negative integer to index slice, got %q", currentPath, key)
			}
			if idx < 0 || idx >= len(typed) {
				return nil, &IndexOutOfRangeError{Path: currentPath, Index: idx, Length: len(typed)}
			}

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +45 to +55
// Keys are interpreted in two ways depending on the type of the current node:
// - For map nodes (map[string]any), the key is used as a literal map key.
// - For slice nodes ([]any), the key must be a non-negative integer literal
// that is used as the index into the slice (e.g. "containers.0.image").
//
// Errors:
// - MissingKeyError when a map key does not exist.
// - IndexOutOfRangeError when a slice index is outside the slice bounds.
// - A plain error when a slice is reached but the next key is not a valid
// non-negative integer, or when the current node is neither a map nor a
// slice but more keys remain.
Comment on lines +220 to +225
errIs: func(err error) bool {
_, ok := err.(*IndexOutOfRangeError)
return ok
},
},
{
@raelga
Copy link
Copy Markdown
Contributor Author

raelga commented May 21, 2026

Closing per architectural clarification from @stevekuznetsov (in the cc thread on this PR and #237) and @geoberle, and as documented in https://github.com/Azure/ARO-HCP/blob/main/docs/configuration.md#limitations ("Avoid using arrays in configuration. Instead, represent arrays as a list of comma separated values"), per-region array configuration is not architecturally supported.

The intended consumer of slice-indexed GetByPath lookups (the dunder placeholder generator walking __foo.0__, __foo.1__, ...) only exists to support arrays in per-region configuration, which the policy disallows.

Closing in favor of #237, which addresses the surviving scalar (boolean / integer / number) dunder-validation case and stays inside the documented configuration limitations. Tracking via AROSLSRE-900.

@raelga raelga closed this May 21, 2026
raelga added a commit to raelga/azure-ARO-Tools that referenced this pull request May 21, 2026
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants