Skip to content

Conversation

@tricktron
Copy link
Contributor

Changes

Follows up on #8490 and fixes all the controller-gen warnings. Now all crds can be generated with the following command without any errors:

controller-gen crd:generateEmbeddedObjectMeta=false paths=./pkg/apis/pipeline/... output:dir=./crds

The main changes are:

  • Remove the duplicated // +listType=atomic annotation from struct fields where the annotation is already set on the type.
  • Create a type alias and set the list type annotation there (mainly with []corev1.Volume.
  • Set the storagversion annotation to the most stable version.

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

  • Has Docs if any changes are user facing, including updates to minimum requirements e.g. Kubernetes version bumps
  • Has Tests included if any functionality added or changed
  • pre-commit Passed
  • Follows the commit message standard
  • Meets the Tekton contributor standards (including functionality, content, code)
  • Has a kind label. You can add one by adding a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
  • Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings). See some examples of good release notes.
  • Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

NONE

@tekton-robot tekton-robot added the release-note-none Denotes a PR that doesnt merit a release note. label Apr 27, 2025
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Apr 27, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: tricktron / name: tricktron (2c9a667)

@tekton-robot tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 27, 2025
@tricktron
Copy link
Contributor Author

/kind cleanup

@tekton-robot tekton-robot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Apr 27, 2025
@tricktron
Copy link
Contributor Author

/assign @pritidesai

@tricktron
Copy link
Contributor Author

@burigolucas and @vdemeester any inputs from your side?

@vdemeester
Copy link
Member

@tricktron thanks for the PR. Seems like it doesn't compile.

 # github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1
Error: pkg/apis/pipeline/v1beta1/taskrun_conversion.go:266:13: rr.ConvertTo undefined (type TaskRunReason has no field or method ConvertTo)
Error: pkg/apis/pipeline/v1beta1/taskrun_conversion.go:319:49: cannot use new (variable of type TaskRunStatus) as TaskRunReason value in argument to append
Error: pkg/apis/pipeline/v1beta1/zz_generated.deepcopy.go:2601:10: cannot use make([]TaskRunStatus, len(*in)) (value of type []TaskRunStatus) as RetriesStatus value in assignment
Error: pkg/apis/pipeline/v1beta1/zz_generated.deepcopy.go:2603:13: (*in)[i].DeepCopyInto undefined (type TaskRunReason has no field or method DeepCopyInto)

@tricktron tricktron force-pushed the fix-controller-annotations branch from d2d75c5 to 5cd0625 Compare May 7, 2025 13:20
@tricktron
Copy link
Contributor Author

@vdemeester I fixed the compile error.

@tricktron tricktron force-pushed the fix-controller-annotations branch from 5cd0625 to db09918 Compare May 12, 2025 12:51
@tricktron
Copy link
Contributor Author

@vdemeester I fixed the error in the hack/verify-codegen.sh ci job.

Copy link
Member

@afrittoli afrittoli left a comment

Choose a reason for hiding this comment

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

/approve

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 12, 2025
@afrittoli
Copy link
Member

Thanks @tricktron - since type definitions are slightly different, you need to re-run the code generation script

@tekton-robot tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 12, 2025
@tricktron
Copy link
Contributor Author

@afrittoli, Thanks for the headsup. I added the new files with are generated with ./hack/update-codegen.sh.

@burigolucas
Copy link
Contributor

burigolucas commented May 12, 2025

Thanks for this PR @tricktron Now that these changes solve the warnings, do we still get response code non zero from controller-gen? The warnings were braking the execution of update-schemas.sh. If we now only get response code 0, I would suggest that we remove the redirect in

paths=$API_PATH/$API_SUBDIR/... > $LOG_FILE 2>&1

@tricktron
Copy link
Contributor Author

tricktron commented May 13, 2025

Thanks for this PR @tricktron Now that these changes solve the warnings, do we still get response code non zero from controller-gen? The warnings were braking the execution of update-schemas.sh. If we now only get response code 0, I would suggest that we remove the redirect in

paths=$API_PATH/$API_SUBDIR/... > $LOG_FILE 2>&1

@burigolucas Good point. Can´t we just replace this whole script with the following command:

go run sigs.k8s.io/controller-tools/cmd/[email protected] crd:generateEmbeddedObjectMeta=false output:dir=./config/300-crds paths=./pkg/apis/...

@burigolucas
Copy link
Contributor

Thanks for this PR @tricktron Now that these changes solve the warnings, do we still get response code non zero from controller-gen? The warnings were braking the execution of update-schemas.sh. If we now only get response code 0, I would suggest that we remove the redirect in

paths=$API_PATH/$API_SUBDIR/... > $LOG_FILE 2>&1

@burigolucas Good point. Can´t we just replace this whole script with the following command:

go run sigs.k8s.io/controller-tools/cmd/[email protected] crd:generateEmbeddedObjectMeta=false output:dir=./config/300-crds paths=./pkg/apis/...

It won't work because the path depends on the API group. The script handles that. What I hope we can do is to get rid off the while loop, assuming there are no warnings returning status code 1 anymore. Please, see the FIXME comment on line 53 about it. The script will be much more clean without the while loop with the retries.

@waveywaves
Copy link
Member

/ok-to-test

@tekton-robot tekton-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label May 13, 2025
@tricktron
Copy link
Contributor Author

@burigolucas I refactored and simplfied the hack/update-schemas.sh script. Have a look 😃.

@tricktron
Copy link
Contributor Author

tricktron commented May 13, 2025

@burigolucas I tried to simplify the hack/update-schemas.sh file but now I am hitting the go panic describe here: kubernetes-sigs/controller-tools#627.

It seems that your while loop somehow protects against this. I have no idea why. Do you?

@tricktron tricktron force-pushed the fix-controller-annotations branch from 0f029ac to 06a7114 Compare May 14, 2025 14:21
@tricktron
Copy link
Contributor Author

@burigolucas Alright, I figured it out. Tekton diverges from the recommended one Go package per group/version kind and uses multiple Go packages per group/version which breaks controller-gen. As a result, we need to generate each CRD in isolation with the correct API package path. I fixed the hack/update-schemas.sh file and added a comment explaining this workaround.

Now all checks in the CI should pass. Could someone retrigger the CI to check?

@tricktron tricktron force-pushed the fix-controller-annotations branch from 06a7114 to 1f0f909 Compare May 20, 2025 16:52
@waveywaves
Copy link
Member

@tricktron can you squash the commits into one and update the commit with a proper commit message, that would be done, after that I can approve it ✅

@tricktron tricktron force-pushed the fix-controller-annotations branch from 1f0f909 to f205608 Compare May 21, 2025 09:15
@tricktron
Copy link
Contributor Author

@waveywaves Sure, I squashed everything into one commit and reworded the commit message.

@tricktron tricktron force-pushed the fix-controller-annotations branch from f205608 to b1e6b37 Compare May 21, 2025 09:25
Changes the following Kubernetes controller annotations so that the
`controller-gen:schemapatch` command works without any errors:

- Removes the duplicated list type annotation from struct fields where
  the annotation is already set on the type.
- Create a type alias and set the list type annotation there (mainly
  with `[]corev1.Volume`).
- Set the `storageversion` annotation to the most stable version.

As a result, we can then simplify the update-schema.sh script.
@tricktron tricktron force-pushed the fix-controller-annotations branch from b1e6b37 to 2c9a667 Compare May 21, 2025 09:26
@waveywaves
Copy link
Member

cc @vdemeester

@tricktron
Copy link
Contributor Author

@vdemeester @waveywaves kind ping.

@waveywaves
Copy link
Member

/retest

@tricktron tricktron requested a review from waveywaves May 30, 2025 16:05
@tricktron
Copy link
Contributor Author

@vdemeester @waveywaves Kind reminder to have another look at this😃.

Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

👼🏼

Copy link
Member

@waveywaves waveywaves left a comment

Choose a reason for hiding this comment

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

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 11, 2025
@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afrittoli, vdemeester, waveywaves

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

The pull request process is described here

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 merged commit 2804278 into tektoncd:main Jun 11, 2025
20 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. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note-none Denotes a PR that doesnt merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

7 participants