Skip to content

fix(hack): stop sync-helm-crds.sh reviving dead manual CRDs - #3842

Merged
tekton-robot merged 2 commits into
tektoncd:mainfrom
jkhelil:fix/sync-helm-crds-drop-manual-crds
Aug 3, 2026
Merged

fix(hack): stop sync-helm-crds.sh reviving dead manual CRDs#3842
tekton-robot merged 2 commits into
tektoncd:mainfrom
jkhelil:fix/sync-helm-crds-drop-manual-crds

Conversation

@jkhelil

@jkhelil jkhelil commented Jul 30, 2026

Copy link
Copy Markdown
Member

Changes

1. hack/sync-helm-crds.sh still regenerated dead manual CRD copies

hack/sync-helm-crds.sh still regenerated the manual per-component CRD
copies under config/base/, config/kubernetes/base/, and
config/openshift/base/ (300-operator_v1alpha1_<component>_crd.yaml).

Commit 1ae0906 ("Cleanup manual CRDs and use generated CRDs in
kustomize") deleted those files and pointed every kustomization.yaml
at config/base/generated-crds/*.yaml directly, but never updated
this script to match. As a result, running make sync-helm-crds (a
documented prerequisite for any CRD-affecting API change) silently
resurrects ~4,700 lines of unreferenced, dead manifests on every use
— nothing in the repo consumes them (verified via git grep across
the tree; the only reference was the script itself).

This surfaced concretely in
#3828, where an unrelated
OpenShiftPipelinesAsCode NetworkPolicy change ballooned into a
~8,900-line diff because the author correctly ran make sync-helm-crds and the stale script did the rest.

This PR drops the dead write_config_crd step (and the now-unused
BASE_DIR/K8S_DIR/OPENSHIFT_DIR variables) so the script only
does what's still needed: assembling the Helm chart CRD bundles
(charts/tekton-operator/templates/{kubernetes,openshift}-crds.yaml)
from config/base/generated-crds/, which remains kustomize's sole
source of truth.

2. TektonConfig.spec.hub.options was implicitly required

While investigating #1, I found that running make generate-crds on
unmodified main (independent of any other PR) produces a
required: [options] entry under spec.hub in
config/base/generated-crds/operator.tekton.dev_tektonconfigs.yaml
the file kustomize applies directly to clusters. That would reject
any TektonConfig CR that doesn't explicitly set spec.hub.options,
a breaking regression for existing users on upgrade.

Root cause: Hub.Options in tektonconfig_types.go was the only
Options AdditionalOptions field across all components missing the
// +optional marker (every other component — Pipeline, Trigger,
Chain, Dashboard, Addon, ManualApprovalGate, Pruner, Scheduler,
MulticlusterProxyAAE, SyncerService — has it). Almost certainly an
oversight from the earlier "fix: options field should be optional in
all components" work.

Fixed by adding +optional/omitempty to match every other
component, then regenerating operator.tekton.dev_tektonconfigs.yaml
and charts/tekton-operator/templates/kubernetes-crds.yaml via
make sync-helm-crds. The regeneration also picks up an unrelated,
already-correct doc fix: the networkPolicy field description now
mentions TektonPipeline alongside TektonTrigger.

charts/tekton-operator/templates/openshift-crds.yaml is
intentionally left untouched: it has independently drifted to bundle
only 3 of the 13 CRDs it should, and re-syncing it pulls in a large,
unrelated diff that deserves its own review/PR.

Verification

  • bash -n hack/sync-helm-crds.sh — syntax OK
  • go build ./pkg/... and go vet ./pkg/apis/operator/v1alpha1/... — clean
  • go test ./pkg/apis/operator/v1alpha1/... — no new failures (3
    pre-existing failures in TestSetPACControllerDefaultSettings*
    remain, caused by an unrelated pipelines-as-code v0.48.0→v0.48.1
    default hub-catalog-type change, already broken on unmodified
    main)
  • Ran the fixed script against main: no config/*/base/*_crd.yaml
    files are recreated, and (before the hub.options fix)
    kubernetes-crds.yaml regenerated byte-for-byte identical to
    what's committed, confirming it was already in sync

Submitter Checklist

Release Notes

NONE

Made with Cursor

hack/sync-helm-crds.sh still regenerated the manual per-component
CRD copies under config/base/, config/kubernetes/base/, and
config/openshift/base/ (300-operator_v1alpha1_<component>_crd.yaml).
Commit 1ae0906 ("Cleanup manual CRDs and use generated CRDs in
kustomize") deleted those files and pointed every kustomization.yaml
at config/base/generated-crds/*.yaml directly, but never updated this
script to match. Running `make sync-helm-crds` therefore silently
resurrected ~4,700 lines of unreferenced manifests on every use.

Drop the write_config_crd step and the now-unused BASE_DIR/K8S_DIR/
OPENSHIFT_DIR variables. The script now only assembles the Helm chart
CRD bundles from config/base/generated-crds/, which remains the sole
source of truth consumed by kustomize.

Signed-off-by: Jawed khelil <jkhelil@redhat.com>
Assisted-by: Claude Sonnet 4.5 (via Cursor)
Co-authored-by: Cursor <cursoragent@cursor.com>
@tekton-robot tekton-robot added the release-note-none Denotes a PR that doesnt merit a release note. label Jul 30, 2026
@tekton-robot tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 30, 2026
@codecov

codecov Bot commented Jul 30, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 25.46%. Comparing base (3d84a8a) to head (2d28821).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3842   +/-   ##
=======================================
  Coverage   25.46%   25.46%           
=======================================
  Files         449      449           
  Lines       23494    23494           
=======================================
  Hits         5982     5982           
  Misses      16823    16823           
  Partials      689      689           
Flag Coverage Δ
unit-tests 25.46% <ø> (ø)

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.

Hub.Options in tektonconfig_types.go was missing the +optional
marker that every sibling component's Options field has (Pipeline,
Trigger, Chain, Dashboard, Addon, ManualApprovalGate, Pruner,
Scheduler, MulticlusterProxyAAE, SyncerService). As a result,
`make generate-crds` (verified by running it against unmodified
main, independent of this branch) produces a `required: [options]`
entry under spec.hub in the generated TektonConfig CRD — the file
kustomize applies directly to clusters. That would reject any
TektonConfig CR that does not explicitly set spec.hub.options, a
breaking regression for existing users on upgrade.

Add +optional and omitempty to match every other component, and
regenerate config/base/generated-crds/operator.tekton.dev_tektonconfigs.yaml
and charts/tekton-operator/templates/kubernetes-crds.yaml via
`make sync-helm-crds`. The regeneration also picks up an unrelated
but already-correct doc fix: the networkPolicy field description
now mentions TektonPipeline alongside TektonTrigger as components
with NetworkPolicy reconciliation implemented.

charts/tekton-operator/templates/openshift-crds.yaml is intentionally
left untouched here: it has independently drifted to bundle only 3
of the 13 CRDs it should, and re-syncing it pulls in a large,
unrelated diff that deserves its own review.

Signed-off-by: Jawed khelil <jkhelil@redhat.com>
Assisted-by: Claude Sonnet 4.5 (via Cursor)
Co-authored-by: Cursor <cursoragent@cursor.com>

@enarha enarha left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve

@tekton-robot

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enarha

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

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

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

Copy link
Copy Markdown
Contributor

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 3, 2026
@tekton-robot
tekton-robot merged commit f2ce2e4 into tektoncd:main Aug 3, 2026
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesnt merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants