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.

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

CRD docs integration #263

Merged
merged 6 commits into from
Oct 13, 2023
Merged

CRD docs integration #263

merged 6 commits into from
Oct 13, 2023

Conversation

jhkrug
Copy link
Contributor

@jhkrug jhkrug commented Oct 9, 2023

Do you like the look of this? I've just added the generated file contents into the page to see what it looks like.

@flavio , @viccuad , @jvanz

@jhkrug jhkrug added the area/documentation Improvements or additions to documentation label Oct 9, 2023
@jhkrug jhkrug requested a review from a team October 9, 2023 11:56
@jhkrug jhkrug self-assigned this Oct 9, 2023
@jhkrug jhkrug requested a review from a team as a code owner October 9, 2023 11:56
@netlify
Copy link

netlify bot commented Oct 9, 2023

Deploy Preview for silly-bunny-8cedd0 ready!

Name Link
🔨 Latest commit 32df535
🔍 Latest deploy log https://app.netlify.com/sites/silly-bunny-8cedd0/deploys/65290f7ae22a670008f9720f
😎 Deploy Preview https://deploy-preview-263--silly-bunny-8cedd0.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@jhkrug jhkrug marked this pull request as draft October 9, 2023 11:57
@viccuad
Copy link
Member

viccuad commented Oct 9, 2023

@jhkrug it's starting to look good, but the matchPolicy file entry breaks the rest of the table.
Compare with: https://doc.crds.dev/github.com/kubewarden/kubewarden-controller/policies.kubewarden.io/ClusterAdmissionPolicy/[email protected]#spec-matchPolicy

@viccuad
Copy link
Member

viccuad commented Oct 9, 2023

This is personal preference, but I wonder if we could not generate the docs for old v1alpha versions, and only show v1, as happens in https://doc.crds.dev/github.com/kubewarden/kubewarden-controller/policies.kubewarden.io/AdmissionPolicy/[email protected] (see top right).

@flavio
Copy link
Member

flavio commented Oct 9, 2023

I think this is a step in the right direction. There are some minor rendering glitches as others pointed out, but I like it!

@jhkrug
Copy link
Contributor Author

jhkrug commented Oct 9, 2023

@jhkrug it's starting to look good, but the matchPolicy file entry breaks the rest of the table. Compare with: https://doc.crds.dev/github.com/kubewarden/kubewarden-controller/policies.kubewarden.io/ClusterAdmissionPolicy/[email protected]#spec-matchPolicy

This because the documentation source contains markdown for a list. A markdown list is not permitted in a markdown table cell. At least not in docusaurus. So, you can't put markdown formatting in the crd source. Plain, vanilla, unformatted text only. It's likely not to work. It works for asciidoc. Which we don't use, sadly perhaps.

@viccuad
Copy link
Member

viccuad commented Oct 9, 2023

Maybe we can generate the crd docs in markdown instead, that could be a possibility too.

@jhkrug
Copy link
Contributor Author

jhkrug commented Oct 9, 2023

Maybe we can generate the crd docs in markdown instead, that could be a possibility too.

That's what I did for the demo. We can't use the default asciidoc.

@jhkrug
Copy link
Contributor Author

jhkrug commented Oct 9, 2023

I can take a look at the docs source file for this and try opening a PR in there.

@jhkrug
Copy link
Contributor Author

jhkrug commented Oct 9, 2023

@jhkrug it's starting to look good, but the matchPolicy file entry breaks the rest of the table. Compare with: https://doc.crds.dev/github.com/kubewarden/kubewarden-controller/policies.kubewarden.io/ClusterAdmissionPolicy/[email protected]#spec-matchPolicy

This because the documentation source contains markdown for a list. A markdown list is not permitted in a markdown table cell. At least not in docusaurus. So, you can't put markdown formatting in the crd source. Plain, vanilla, unformatted text only. It's likely not to work. It works for asciidoc. Which we don't use, sadly perhaps.

You can use a html list in policy_types.go and it works as well. So, that's another option.

@jhkrug
Copy link
Contributor Author

jhkrug commented Oct 10, 2023

So, it appears that the most you can get away with regarding docstring formatting is a html encoded list and list items. And I'd discourage that as well. Too prone to error and complications.

Some of the docstrings have code blocks in them. They don't work. I'd suggest an iteration of the docstrings. There is perhaps too much material in there. Keep it as short as possible, with links to doc pages as necessary for further explanations.

So why does it currently work? Well, different tool chains, but it doesn't really, particularly the namespaceSelector at the bottom.

image

@jhkrug jhkrug mentioned this pull request Oct 10, 2023
1 task
@jhkrug jhkrug changed the title CRD docs integration - trial - don't merge yet. CRD docs integration Oct 11, 2023
@jhkrug jhkrug marked this pull request as ready for review October 11, 2023 15:07
Copy link
Member

@viccuad viccuad left a comment

Choose a reason for hiding this comment

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

LGTM!
Super happy with this! This section was also the last one not accessible offline, now we can ship the docs together with the helm-charts, make them available even on airgap, or provide them in the UI!

@flavio
Copy link
Member

flavio commented Oct 12, 2023

This looks really good to me.

What about adding a more explicit link to that, right now this is kinda buried under sub-menus on the left sidebar.

I propose to do something like that:

image

If you're fine with that I can add this small change to this branch

@viccuad
Copy link
Member

viccuad commented Oct 12, 2023

I'm ok with making the CRDs more explicit, maybe just under "Reference docs" and not "Reference docs -> Operator Manual".
I'm also ok linking to the CRDs docs from the CRD names in the quickstart (I don't understand much the shared screenshot).

@flavio
Copy link
Member

flavio commented Oct 13, 2023

I'm also ok linking to the CRDs docs from the CRD names in the quickstart (I don't understand much the shared screenshot).

Forgive me, I wasn't really explicit about the change I made. I've just added the new "TIP" section, the one in green.

@jhkrug jhkrug requested review from viccuad and flavio October 13, 2023 09:37
Copy link
Member

@flavio flavio 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 the new changes!

@jhkrug jhkrug merged commit 81327b3 into kubewarden:main Oct 13, 2023
6 checks passed
@jhkrug jhkrug deleted the crd-integration branch October 13, 2023 12:58
@jhkrug jhkrug linked an issue Oct 13, 2023 that may be closed by this pull request
1 task
@jhkrug jhkrug mentioned this pull request Oct 24, 2023
74 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/documentation Improvements or additions to documentation
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

CRD documentation
3 participants