feat(manualapprovalgate): integrate MAG as TektonConfig child component by pratap0007 · Pull Request #3762 · tektoncd/operator

pratap0007 · 2026-07-20T06:30:07Z

Changes

Add ManualApproval field to TektonConfig spec so that TektonConfig manages the ManualApprovalGate CR lifecycle including ownerRef and platform-data-hash propagation for TLS profile changes.

Disabled by default on fresh installs
On upgrade, standalone MAG CRs (no ownerRef) are adopted under TektonConfig instead of being deleted
Add validation for spec.manualApproval.options
Add unit tests for create, delete, ownerRef migration, and platform-data-hash annotation sync

Fixes #3656

Assisted-by: Claude Opus 4.6 (via Claude Code)

Testing

Verified fresh installation with MAG enabled and disabled; both scenarios work as expected.
Verified the upgrade scenario with an existing MAG installation that is adopted by the TektonConfig; the upgrade works as expected.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Run make test lint before submitting a PR
Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices

See the contribution guide for more details.

Release Notes

Add ManualApproval component to the TektonConfig to manage the ManualApprovalGate CR lifecycle.

codecov-commenter · 2026-07-20T06:47:04Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 26.39%. Comparing base (818e85d) to head (4572a2c).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3762      +/-   ##
==========================================
+ Coverage   26.13%   26.39%   +0.26%     
==========================================
  Files         465      467       +2     
  Lines       24930    25085     +155     
==========================================
+ Hits         6516     6622     +106     
- Misses      17694    17736      +42     
- Partials      720      727       +7

Flag	Coverage Δ
unit-tests	`26.39% <ø> (+0.26%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot

Pull request overview

Integrates the ManualApprovalGate (MAG) component into the TektonConfig reconciliation flow so TektonConfig can create/adopt/delete the MAG CR and propagate platform-data-hash changes, aligning MAG with how other Tekton components are managed.

Changes:

Add spec.manualApproval to TektonConfigSpec with defaulting + validation wiring.
Introduce shared manualapprovalgate helper package to ensure/create/update/delete the MAG CR (including ownerRef and platform-data-hash propagation).
Add unit tests covering MAG create/delete, ownerRef migration, and platform-data-hash propagation.

Reviewed changes

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

Show a summary per file

File	Description
pkg/reconciler/shared/tektonconfig/tektonconfig.go	Wire MAG lifecycle into TektonConfig reconcile/finalize and propagate platform-data-hash.
pkg/reconciler/shared/tektonconfig/manualapprovalgate/manualapprovalgate.go	New shared helper functions to ensure/update/delete MAG CR.
pkg/reconciler/shared/tektonconfig/manualapprovalgate/manualapprovalgate_test.go	Unit tests for MAG helper behaviors (create/delete/migration/platform hash).
pkg/apis/operator/v1alpha1/zz_generated.deepcopy.go	Generated deepcopy updates for new `ManualApproval.Disabled` pointer + TektonConfigSpec field.
pkg/apis/operator/v1alpha1/tektonconfig_validation.go	Add validation for `spec.manualApproval.options`.
pkg/apis/operator/v1alpha1/tektonconfig_types.go	Add `ManualApproval` field to TektonConfig spec.
pkg/apis/operator/v1alpha1/tektonconfig_defaults.go	Default `ManualApproval` to disabled on fresh installs.
pkg/apis/operator/v1alpha1/manualapprovalgate_types.go	Add `ManualApproval.Disabled` + helpers (`setDefaults`, `IsDisabled`).

Files not reviewed (1)

pkg/apis/operator/v1alpha1/zz_generated.deepcopy.go: Generated file

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

anithapriyanatarajan · 2026-07-30T11:28:48Z

 	tektonConfigreconciler "github.com/tektoncd/operator/pkg/client/injection/reconciler/operator/v1alpha1/tektonconfig"
 	"github.com/tektoncd/operator/pkg/reconciler/common"
 	"github.com/tektoncd/operator/pkg/reconciler/shared/tektonconfig/chain"
+	"github.com/tektoncd/operator/pkg/reconciler/shared/tektonconfig/manualapprovalgate"


ManualApprovalGate is not registered in webhook.go's types map. This means SetDefaults and Validate are never called when users create or edit MAG CRs directly. Every component that has a standalone reconciler and is in AGENTS.md's "pattern references" table is registered there (TektonPipeline, TektonChain, TektonResult, TektonPruner, etc.). Please add MAG to the types map.

anithapriyanatarajan · 2026-07-30T11:31:41Z

@pratap0007 please update docs https://github.com/tektoncd/operator/blob/main/docs/TektonConfig.md to indicate that with this PR MAG configs can now be managed with tektonconfig. Thank you.

pratap0007 · 2026-07-31T14:03:23Z

@anithapriyanatarajan Thank you for the review, I've updated the PR and could you please take another look

jkhelil · 2026-08-04T08:29:49Z

+	magEnabled := !tc.Spec.ManualApproval.IsDisabled()
+	if !magEnabled {
+		existingMAG, err := manualapprovalgate.GetManualApprovalGate(ctx, r.operatorClientSet.OperatorV1alpha1().ManualApprovalGates(), v1alpha1.ManualApprovalGates)
+		if err == nil && (tc.Spec.ManualApproval.Disabled == nil || len(existingMAG.OwnerReferences) == 0) {


Any error other than "not found" (e.g. a transient API error, RBAC issue) is silently treated the same as "no standalone CR found" with no log line. Consider logging non-NotFound errors so real problems (e.g. missing RBAC for manualapprovalgates) aren't invisible.

jkhelil · 2026-08-04T08:38:34Z

+	magEnabled := !tc.Spec.ManualApproval.IsDisabled()
+	if !magEnabled {
+		existingMAG, err := manualapprovalgate.GetManualApprovalGate(ctx, r.operatorClientSet.OperatorV1alpha1().ManualApprovalGates(), v1alpha1.ManualApprovalGates)
+		if err == nil && (tc.Spec.ManualApproval.Disabled == nil || len(existingMAG.OwnerReferences) == 0) {


Step 1 — Fresh upgrade (day 0) A cluster already has a standalone ManualApprovalGate CR (no owner) from before this feature existed. The TektonConfig CR is old too, so its stored spec has no manualApproval field at all → tc.Spec.ManualApproval.Disabled is nil (Go zero value, since the field never existed in that stored object).

Code says:

if tc.Spec.ManualApproval.Disabled == nil || len(existingMAG.OwnerReferences) == 0 {
magEnabled = true // adopt it
}
Disabled == nil is true → it adopts the MAG CR (sets ownerRef on it). Good, this works as intended.

Step 2 — Someone edits TektonConfig later (day 30) Say an admin changes an unrelated field (e.g. targetNamespace, or a GitOps tool just re-applies the same YAML). That triggers a normal Kubernetes Update call, which goes through the mutating webhook. The webhook runs SetDefaults(), and SetDefaults always fills in Disabled: true if it's nil. So now the stored TektonConfig spec permanently has manualApproval.disabled: true written into it — even though nobody touched that field on purpose.

Step 3 — Next reconcile (day 30, same moment) Now check the same condition again:

tc.Spec.ManualApproval.Disabled == nil → false (it's true now, not nil anymore)
len(existingMAG.OwnerReferences) == 0 → false (we already set the ownerRef back in Step 1)
Both are false, so magEnabled stays false. The reconciler thinks "user wants MAG disabled" and calls EnsureManualApprovalGateCRNotExists, which deletes the MAG CR — even though it was already adopted and working fine, and nobody explicitly asked to disable it.

The fix, simply: stop using Disabled == nil as the signal. Once a MAG CR already has the TektonConfig's ownerRef on it, that's proof adoption already happened — the code should just trust the current explicit Disabled value from then on (and never re-trigger deletion just because defaulting ran). Only use "no ownerRef" as the one-time trigger for adopting a legacy CR.
May be a fix would be
magEnabled := !tc.Spec.ManualApproval.IsDisabled()
if !magEnabled {
// If a standalone MAG CR exists with no ownerRef (pre-upgrade legacy CR),
// adopt it now. Once adopted (ownerRef set), the user's explicit Disabled
// value takes over — we never force magEnabled=true again after that.
existingMAG, err := manualapprovalgate.GetManualApprovalGate(ctx, ...)
if err == nil && len(existingMAG.OwnerReferences) == 0 {
logger.Debug("Found standalone ManualApprovalGate CR from previous version, adopting under TektonConfig")
magEnabled = true
}
}

pratap0007 · 2026-08-06T15:52:58Z

/retest

pratap0007 · 2026-08-07T06:34:30Z

/retest

Add ManualApproval field to TektonConfig spec so that TektonConfig manages the ManualApprovalGate CR lifecycle including ownerRef and platform-data-hash propagation for TLS profile changes. - Disabled by default on fresh installs - On upgrade, standalone MAG CRs (no ownerRef) are adopted under TektonConfig instead of being deleted - Add validation for spec.manualApproval.options - Add unit tests for create, delete, ownerRef migration, and platform-data-hash annotation sync Fixes tektoncd#3656 Signed-off-by: Shiv Verma <shverma@redhat.com> Assisted-by: Claude Opus 4.6 (via Claude Code)

pratap0007 · 2026-08-10T06:46:57Z

/retest

jkhelil · 2026-08-10T07:08:05Z

/approve

jkhelil · 2026-08-10T07:08:19Z

/approve cancel

tekton-robot · 2026-08-10T07:08:26Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please ask for approval from pratap0007 after the PR has been reviewed.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

jkhelil · 2026-08-10T07:27:17Z

+	// Ensure ManualApprovalGate CR
+	// If a standalone MAG CR exists (no ownerRef, from a previous version),
+	// adopt its config into TektonConfig and enable it.
+	if tc.Spec.ManualApproval.IsDisabled() {


Transient GetManualApprovalGate error silently falls through to the MAG deletion branch, potentially destroying a standalone pre-upgrade MAG CR before it has been adopted.
The upgrade scenario to trace:

Cluster has a standalone MAG CR (no ownerRef) from a pre-integration version.

Operator is upgraded; preUpgradeManualApprovalGate runs but the MAG API temporarily returns a 503 (or the RBAC permission for get on manualapprovalgates is slow to propagate).
GetManualApprovalGate returns a non-NotFound error → err == nil is false → adoption block does NOT return early.

Code falls through to if !tc.Spec.ManualApproval.IsDisabled() → false → else → - EnsureManualApprovalGateCRNotExists → deletes the standalone MAG CR.
The user's pre-existing MAG installation is destroyed without ever being adopted

Fix
if tc.Spec.ManualApproval.IsDisabled() {
existingMAG, err := manualapprovalgate.GetManualApprovalGate(ctx, ...)
if err != nil {
if !apierrs.IsNotFound(err) {
// Transient error — can't determine whether a standalone MAG
// exists, so don't proceed to the delete branch.
logger.Warnw("Transient error checking for standalone ManualApprovalGate CR, will retry", "error", err)
return v1alpha1.REQUEUE_EVENT_AFTER
}
// NotFound — no standalone MAG, fall through to delete branch (correct)
} else if len(existingMAG.OwnerReferences) == 0 {
// Adopt...
return v1alpha1.REQUEUE_EVENT_AFTER
}
}

Thank you for reviewing and identifying these edge cases. I will update with the above fix.

jkhelil · 2026-08-10T07:33:43Z

@pratap0007 There is no E2E test covering the adoption scenario at all. The ideal deliverable from the author is a new sub-test in test/e2e/common/06_manualapprovalgatedeployment_test.go (or a new 06_mag_via_tektonconfig_test.go) that:

Creates a standalone MAG CR (no ownerRef, simulating pre-upgrade state).
Triggers a TektonConfig reconcile.
Asserts TektonConfig.Spec.ManualApproval.Disabled == false (adoption happened).
Asserts ManualApprovalGate.OwnerReferences contains TektonConfig.
Asserts MAG reaches Ready.
As part of upgrade tests

tekton-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jul 20, 2026

tekton-robot requested review from anithapriyanatarajan and pramodbindal July 20, 2026 06:30

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 20, 2026

jkhelil requested a review from Copilot July 24, 2026 09:03

Copilot started reviewing on behalf of jkhelil July 24, 2026 09:03 View session

Copilot AI reviewed Jul 24, 2026

View reviewed changes

Comment thread pkg/reconciler/shared/tektonconfig/tektonconfig.go Outdated

Comment thread pkg/reconciler/shared/tektonconfig/manualapprovalgate/manualapprovalgate.go Outdated

pratap0007 force-pushed the add-mag-in-tektonconfig branch 3 times, most recently from 0b2d99e to 3f8d01b Compare July 29, 2026 14:45

tekton-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 29, 2026

anithapriyanatarajan reviewed Jul 30, 2026

View reviewed changes

Comment thread pkg/apis/operator/v1alpha1/manualapprovalgate_types.go Outdated

anithapriyanatarajan reviewed Jul 30, 2026

View reviewed changes

anithapriyanatarajan requested changes Jul 30, 2026

View reviewed changes

pratap0007 force-pushed the add-mag-in-tektonconfig branch from 3f8d01b to a41cb3f Compare July 31, 2026 14:00

jkhelil reviewed Aug 4, 2026

View reviewed changes

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 5, 2026

pratap0007 force-pushed the add-mag-in-tektonconfig branch from a41cb3f to 62b8a3a Compare August 6, 2026 15:41

tekton-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 6, 2026

pratap0007 force-pushed the add-mag-in-tektonconfig branch from 62b8a3a to 4572a2c Compare August 7, 2026 08:33

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 10, 2026

tekton-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 10, 2026

jkhelil reviewed Aug 10, 2026

View reviewed changes

Uh oh!

Conversation

pratap0007 commented Jul 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Testing

Submitter Checklist

Release Notes

Uh oh!

codecov-commenter commented Jul 20, 2026 • edited by codecov Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

anithapriyanatarajan Jul 30, 2026

Choose a reason for hiding this comment

Uh oh!

anithapriyanatarajan commented Jul 30, 2026

Uh oh!

pratap0007 commented Jul 31, 2026

Uh oh!

jkhelil Aug 4, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jkhelil Aug 4, 2026

Choose a reason for hiding this comment

Uh oh!

pratap0007 commented Aug 6, 2026

Uh oh!

pratap0007 commented Aug 7, 2026

Uh oh!

pratap0007 commented Aug 10, 2026

Uh oh!

jkhelil commented Aug 10, 2026

Uh oh!

jkhelil commented Aug 10, 2026

Uh oh!

tekton-robot commented Aug 10, 2026

Uh oh!

jkhelil Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

pratap0007 Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

jkhelil commented Aug 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

pratap0007 commented Jul 20, 2026 •

edited

Loading

codecov-commenter commented Jul 20, 2026 •

edited by codecov Bot

Loading