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 List Generation #25910

Merged
merged 1 commit into from Jun 12, 2023
Merged

CRD List Generation #25910

merged 1 commit into from Jun 12, 2023

Conversation

dhawton
Copy link
Contributor

@dhawton dhawton commented Jun 5, 2023

Please ensure your pull request adheres to the following guidelines:

  • For first time contributors, read Submitting a pull request
  • All code is covered by unit and/or runtime tests where feasible.
  • All commits contain a well written commit description including a title,
    description and a Fixes: #XXX line if the commit addresses a particular
    GitHub issue.
  • If your commit description contains a Fixes: <commit-id> tag, then
    please add the commit author[s] as reviewer[s] to this issue.
  • All commits are signed off. See the section Developer’s Certificate of Origin
  • Provide a title or release-note blurb suitable for the release notes.
  • Are you a user of Cilium? Please add yourself to the Users doc
  • Thanks for contributing!

Replaces a manually edited list of CRDs with an automatically generated and included RST.

Running make generate-crd-docs in the root project or make update-crdlist in Documentation will run the crdlistgen tool. This tool rebuilds the Documentation/crdlist.rst file by looking at the pkg/k8s/apis/cilium.io/client package's CRD List. It will also scan the rst files of Documentation to see if the CRD is being used as a header anywhere to generate a link to it.

Fixes: #25750

@dhawton dhawton requested review from a team as code owners June 5, 2023 16:20
@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 Jun 5, 2023
@dhawton dhawton requested a review from qmonnet June 5, 2023 16:20
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Jun 5, 2023
@dhawton dhawton force-pushed the dhawton/crd-list-generator branch 2 times, most recently from 81336f2 to f0425a8 Compare June 5, 2023 17:23
qmonnet
qmonnet requested changes Jun 6, 2023
View reviewed changes
Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

Preview at https://deploy-preview-25910--docs-cilium-io.netlify.app/internals/cilium_operator.html#crd-registration: it works great, thanks for this!

There's the CI check missing (see below), but looks good otherwise.

Documentation/Makefile Show resolved Hide resolved
@qmonnet qmonnet added area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. release-note/misc This PR makes changes that have no direct user impact. labels Jun 6, 2023
@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 Jun 6, 2023
@dhawton dhawton force-pushed the dhawton/crd-list-generator branch 3 times, most recently from becb089 to 3342a57 Compare June 6, 2023 15:46
@tklauser
Copy link
Member

tklauser commented Jun 6, 2023

PR #25824 will introduce another CRD (CiliumPodIPPool), so we'll need to coordinate in which order the two PRs are merged.

@dhawton dhawton force-pushed the dhawton/crd-list-generator branch 2 times, most recently from 2a58d87 to 83906b2 Compare June 6, 2023 22:38
@dhawton dhawton requested review from tklauser and qmonnet June 7, 2023 02:15
qmonnet
qmonnet reviewed Jun 7, 2023
View reviewed changes
Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

Thanks for adding the check!

I'm thinking of additional details now, apologies for missing them on the first pass.

  • Can we mark Documentation/crdlist.rst as a generated file to GitHub, so that changes are folded by default in the diffs? We need a new entry for the file in .gitattributes for that, just like for helm-values.rst for example.

  • Can we also remove ownership for this generated file in CODEOWNERS, to avoid team docs-structure to be pulled for review each time the list changes? Again, see the example of helm-values.rst in CODEOWNERS. Given that CI should catch missing updates, we shouldn't need manual validation for this file.

@qmonnet
Copy link
Member

qmonnet commented Jun 7, 2023

Please, also rebase your PR on main to address the merge conflict with pkg/k8s/apis/cilium.io/client/register.go.

@dhawton dhawton force-pushed the dhawton/crd-list-generator branch from 83906b2 to f54cfec Compare June 7, 2023 14:37
@dhawton dhawton requested a review from a team as a code owner June 7, 2023 14:37
@dhawton dhawton force-pushed the dhawton/crd-list-generator branch from f54cfec to 6a8102b Compare June 7, 2023 14:48
qmonnet
qmonnet approved these changes Jun 7, 2023
View reviewed changes
Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

I just had the time to observe that the CI check works as expected :) Thanks a lot!

tklauser
tklauser approved these changes Jun 7, 2023
View reviewed changes
Copy link
Member

@tklauser tklauser left a comment

Choose a reason for hiding this comment

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

Thanks!

@tklauser
Copy link
Member

tklauser commented Jun 7, 2023

/test

@dhawton dhawton force-pushed the dhawton/crd-list-generator branch from 6a8102b to f523c67 Compare June 8, 2023 00:03
@tklauser
Copy link
Member

tklauser commented Jun 8, 2023 •
edited by maintainer-s-little-helper bot

/test

Job 'Cilium-PR-K8s-1.26-kernel-net-next' hit: #25958 (85.99% similarity)

@maintainer-s-little-helper maintainer-s-little-helper bot mentioned this pull request Jun 8, 2023
Closed
@tklauser
Copy link
Member

tklauser commented Jun 8, 2023 •
edited by maintainer-s-little-helper bot

/test-1.26-net-next

Job 'Cilium-PR-K8s-1.26-kernel-net-next' hit: #25958 (94.11% similarity)

@dhawton dhawton force-pushed the dhawton/crd-list-generator branch from f523c67 to 3b1d6aa Compare June 8, 2023 20:17
@dhawton
CRD List Generation
3d5725a 
Running `make generate-crd-docs` in the root project or `make update-crdlist` in Documentation will run the crdlistgen tool. This tool rebuilds the Documentation/crdlist.rst
file by looking at the pkg/k8s/apis/cilium.io/client package's CRD List. It will also scan the rst files of Documentation to see if the CRD is being used as a header anywhere
to generate a link to it.

Signed-off-by: Daniel Hawton <daniel.hawton@solo.io>
@dhawton dhawton force-pushed the dhawton/crd-list-generator branch from 3b1d6aa to 3d5725a Compare June 9, 2023 15:40
@qmonnet
Copy link
Member

qmonnet commented Jun 9, 2023 •
edited by maintainer-s-little-helper bot

/test

Job 'Cilium-PR-K8s-1.25-kernel-4.19' failed:

Click to show.

Test Name

K8sAgentPolicyTest Multi-node policy test with L7 policy using connectivity-check to check datapath

Failure Output

FAIL: connectivity-check pods are not ready after timeout

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.25-kernel-4.19/605/

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.25-kernel-4.19 so I can create one.

Then please upload the Jenkins artifacts to that issue.

This was referenced Jun 12, 2023
Closed
Closed
@qmonnet
Copy link
Member

qmonnet commented Jun 12, 2023

/test-1.25-4.19

@qmonnet qmonnet mentioned this pull request Jun 12, 2023
Closed
@qmonnet
Copy link
Member

qmonnet commented Jun 12, 2023

/test-1.25-4.19

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 12, 2023
@qmonnet qmonnet merged commit 50f72e8 into cilium:main Jun 12, 2023
63 checks passed
@dhawton dhawton deleted the dhawton/crd-list-generator branch June 14, 2023 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tklauser tklauser tklauser approved these changes

@youngnick youngnick youngnick approved these changes

@qmonnet qmonnet qmonnet approved these changes

Assignees
No one assigned
Labels
area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. kind/community-contribution This was a contribution made by a community member. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

docs: Automate the listing of CRDs
4 participants
@dhawton @tklauser @qmonnet @youngnick