Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

crd: Add categories for cilium CRDs. #17162

Merged
merged 2 commits into from Oct 18, 2021
Merged

crd: Add categories for cilium CRDs. #17162

merged 2 commits into from Oct 18, 2021

Conversation

sayboras
Copy link
Member

@sayboras sayboras commented Aug 14, 2021 •
edited

This commit is to add categories for cilium CRDs, so that users can
easily query cilium CRs with kubectl commands (e.g. kubectl get
cilium or kubectl get ciliumpolicy). Two categories (e.g. cilium and
ciliumpolicy) are added to start with.

Signed-off-by: Tam Mach sayboras@yahoo.com

Related slack conversation

crd: Add categories for cilium CRDs

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Aug 14, 2021
@sayboras sayboras added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Aug 14, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Aug 14, 2021
@sayboras sayboras added the area/misc Impacts miscellaneous areas of the code not otherwise owned by another area. label Aug 14, 2021
@sayboras sayboras marked this pull request as ready for review August 14, 2021 08:59
@sayboras sayboras requested a review from a team as a code owner August 14, 2021 08:59
@sayboras sayboras requested review from a team and nathanjsweet August 14, 2021 08:59
nathanjsweet
nathanjsweet approved these changes Aug 16, 2021
View reviewed changes
Copy link
Member

@nathanjsweet nathanjsweet left a comment

Choose a reason for hiding this comment

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

It would be nice if we could add some CI to ensure that future crds do not forget to add the categories field.

@sayboras
Copy link
Member Author

It would be nice if we could add some CI to ensure that future crds do not forget to add the categories field.

Sure, CI has evolved a lot since last time I take a look at it. Will do it in separate PR.

@sayboras
Copy link
Member Author

I don't think the full CI is required, might need input here.

@aanm
Copy link
Member

aanm commented Aug 18, 2021

@sayboras I think a linter that checks for the existence of "categories" in that line should be enough?

@sayboras
Copy link
Member Author

I think a linter that checks for the existence of "categories" in that line should be enough?

Sure, I just add a simple check in second commit, let me know if this is what you have in mind. Manual testing was done by removing one category in ciliumclusterwidenetworkpolicies CRD.

$ make manifests
cd "./vendor/sigs.k8s.io/controller-tools/cmd/controller-gen" && \
go run ./... "crd:crdVersions=v1" paths="/home/tammach/go/src/github.com/cilium/cilium/pkg/k8s/apis/cilium.io/v2;/home/tammach/go/src/github.com/cilium/cilium/pkg/k8s/apis/cilium.io/v2alpha1" output:crd:artifacts:config="/tmp/tmp.WavdeFMNZO";
go run ./tools/categorycheck "/tmp/tmp.WavdeFMNZO"
2021/08/29 18:54:30 CRD categories missing for ciliumclusterwidenetworkpolicies.cilium.io
exit status 1
make: *** [Makefile:303: manifests] Error 1

@sayboras sayboras requested review from a team as code owners August 29, 2021 09:00
@sayboras sayboras requested a review from qmonnet August 29, 2021 09:01
@sayboras sayboras force-pushed the sayboras/add-categories branch 4 times, most recently from 3aaf861 to 176ef8c Compare August 30, 2021 00:05
sayboras
sayboras commented Aug 30, 2021
View reviewed changes
tools/crdcheck/main.go
}

for _, f := range allChecks {
if err = f(crd); err != nil {
Copy link
Member Author

Choose a reason for hiding this comment

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

currently, we have only one check, so i didn't use multi error here. Let's see how it's going.

@aanm
Copy link
Member

aanm commented Sep 15, 2021 •
edited by maintainer-s-little-helper bot

test-me-please

Job 'Cilium-PR-K8s-1.19-kernel-5.4' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sDatapathConfig Host firewall With VXLAN and endpoint routes

Failure Output

FAIL: Failed to reach 192.168.36.11:80 from testserver-host-68t6r

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-1.19-kernel-5.4 so I can create a new GitHub issue to track it.

Job 'Cilium-PR-K8s-1.21-kernel-4.9' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sPolicyTest Basic Test Traffic redirections to proxy Tests proxy visibility interactions with policy lifecycle operations

Failure Output

FAIL: traffic was not redirected to the proxy when it should have been

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-1.21-kernel-4.9 so I can create a new GitHub issue to track it.

@aanm
Copy link
Member

aanm commented Oct 2, 2021 •
edited

/test-1.16-netnext https://jenkins.cilium.io/job/Cilium-PR-K8s-1.16-net-next/1558/

@aanm aanm self-requested a review October 2, 2021 12:59
@aanm
Copy link
Member

aanm commented Oct 4, 2021 •
edited

/test-net-next https://jenkins.cilium.io/job/Cilium-PR-K8s-1.16-net-next/1588/

@aanm
Copy link
Member

aanm commented Oct 11, 2021

/test-net-next

1 similar comment
@aanm
Copy link
Member

aanm commented Oct 12, 2021

/test-net-next

@sayboras
crd: Add categories for cilium CRDs.
4615574 
This commit is to add categories for cilium CRDs, so that users can
easily query cilium CRs with kubectl commands (e.g. kubectl get
cilium or kubectl get ciliumpolicy). Two categories (e.g. cilium and
ciliumpolicy) are added to start with.

Signed-off-by: Tam Mach <sayboras@yahoo.com>
@sayboras
build(crds): Add check for CRDs
0e3fd11 
This commit is to add a simple check for generated CRDs to make sure
that all CRDs must have category field specified, also, it must have
cilium category.

Signed-off-by: Tam Mach <sayboras@yahoo.com>
@sayboras
Copy link
Member Author

/test-net-next

@aanm aanm merged commit a31082a into cilium:master Oct 18, 2021
@sayboras sayboras deleted the sayboras/add-categories branch January 10, 2022 06:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nathanjsweet nathanjsweet nathanjsweet approved these changes

@aanm aanm aanm approved these changes

@qmonnet qmonnet qmonnet approved these changes

Assignees
No one assigned
Labels
area/misc Impacts miscellaneous areas of the code not otherwise owned by another area. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@sayboras @aanm @nathanjsweet @qmonnet