feat(matrix): implement naming for TaskRuns and PipelineRuns#10297
feat(matrix): implement naming for TaskRuns and PipelineRuns#10297MayorFaj wants to merge 3 commits into
Conversation
…trix.include[].name
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because no GitHub Actions runner was available. Make sure your repository has a runner available to run Copilot's review, or add a copilot-setup-steps.yml file specifying one with the runs-on attribute. See the docs for more details.
Implements readable naming for Matrix-fanned child resources by using matrix.include[].name as the suffix when the matrix is include-only and names are valid/unique, while preserving numeric indexing as a fallback.
Changes:
- Add
Matrix.IncludeNames()to derive eligible include-name suffixes (valid DNS1123 label, non-empty, unique, include-only). - Update run-name generation to optionally use include-name suffixes for TaskRuns/CustomRuns/child PipelineRuns.
- Add/adjust unit + integration tests and user docs/example to cover naming behavior, truncation, and ordering.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
pkg/apis/pipeline/v1/matrix_types.go |
Adds IncludeNames() eligibility/validation for include-only matrices. |
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go |
Threads include-name suffixes into child name generation logic. |
pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go |
Expands tests for include-name suffixes, truncation, and stable ordering. |
test/matrix_test.go |
Adds integration coverage verifying child taskrun naming/displayName behavior. |
pkg/reconciler/pipelinerun/pipelinerun_test.go |
Updates expected TaskRun/childReference names to match include-name suffixing. |
docs/matrix.md |
Documents naming rules, eligibility conditions, and truncation behavior. |
examples/v1/pipelineruns/beta/pipelinerun-with-matrix-include-names.yaml |
Adds an example demonstrating include-only naming behavior. |
pkg/apis/pipeline/v1/matrix_types_test.go |
Adds unit tests for Matrix.IncludeNames() eligibility rules. |
Comments suppressed due to low confidence (2)
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go:1
for i := range numberOfRunsrelies on Go's newer 'range over int' feature (introduced in Go 1.22). If this repository/toolchain isn’t guaranteed to be >= 1.22, this will not compile. To avoid toolchain coupling, use a classic indexed loop (for i := 0; i < numberOfRuns; i++) or explicitly ensure/enforce the minimum Go version/toolchain in the repo.
/*
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go:1
useIncludeNamesonly checks slice length equality, but does not guard against empty strings insideincludeNames. If an empty name is ever passed (e.g., from a future caller not usingMatrix.IncludeNames()), this can generate invalid Kubernetes names (trailing '-') or shift behavior in unexpected ways. Consider strengthening the eligibility check (e.g., require allincludeNames[i] != \"\", and possibly DNS-label validation) before using include-name suffixing; otherwise fall back to numeric indexing.
/*
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: MayorFaj <mayorfaj@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Changes
Fixes #10054.
By default, child
TaskRuns/PipelineRuns/Runsfanned out from aMatrixarenamed with a numeric index suffix (
<pipelineRunName>-<pipelineTaskName>-0), whichmakes it hard to tell which combination a child corresponds to.
This PR uses the
matrix.include[].namevalue as the suffix instead(
<pipelineRunName>-<pipelineTaskName>-<includeName>) when all of the following hold:Matrixis include-only (matrix.includeset, nomatrix.params).includeentry has a non-emptyname.nameis a valid DNS label.namevalues are unique.Otherwise it falls back to numeric indexing, so the change is fully backward compatible.
Names exceeding the 63-character limit are truncated (and fall back to the hashed name
when the include name can't be kept intact).
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
/kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes
Change Type