fix: prevent Chain and Result installation on lite profile#3341
fix: prevent Chain and Result installation on lite profile#3341alliasgher wants to merge 1 commit intotektoncd:mainfrom
Conversation
The lite profile should only install TektonPipelines, but the
reconciler was creating TektonChain and TektonResult CRs because
their conditions only checked .Spec.Chain.Disabled (default: false)
without also checking the profile.
This follows the same pattern already used for TektonTrigger, which
correctly gates on both the disabled flag AND the profile:
if !tc.Spec.Trigger.Disabled && (tc.Spec.Profile == ProfileAll || tc.Spec.Profile == ProfileBasic)
Apply the same profile check to Chain and Result so they are only
created for the "all" and "basic" profiles.
Fixes tektoncd#2781
```release-note
NONE
```
|
[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 |
|
/assign @savitaashture |
|
/ok-to-test |
|
The e2e test Some analysis on why my fix exposes this:
This is a real underlying issue in the Result reconciliation/transition flow, but my fix is what surfaced it. Two ways forward:
I'd appreciate guidance from @khrm / @savitaashture on the preferred path. Happy to investigate the Result reconciler stuck-transition issue as a follow-up if option (1) is preferred. |
|
@alliasgher Thank you for this PR. Would recommend to have the CI passing before the PR is merged. Hence, would it be possible to proceed with your proposed first option in last comment: Land this fix as-is and treat the transition issue as a separate bug to fix in the Result reconciler.. If you have the bandwidth, please raise a new issue and submit a PR for that. Once merged, we can rerun the tests on this PR. If bandwidth is tight, even a GitHub issue to capture the problem would be helpful. Very keen to get this PR merged, but want to do it the right way. Feel free to flag any concerns. Thanks again! |
|
/hold |
|
Filed #3377 to capture the TektonResult |
Changes
Fix the
liteprofile installing TektonChain and TektonResult components when it should only install TektonPipelines.Root Cause
The TektonTrigger section correctly gates creation on both the disabled flag AND the profile:
But the TektonChain and TektonResult sections only checked the disabled flag:
Since
Chain.DisabledandResult.Disableddefault tofalse, the lite profile would create these components, then the FinalizeKind path would try to delete them, causing a reconciliation loop.Fix
Add the same
ProfileAll || ProfileBasiccheck to both Chain and Result conditions, matching the existing Trigger pattern.Verification
go build ./...passesFixes #2781