-
Notifications
You must be signed in to change notification settings - Fork 261
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
📖 Add API reference documentation generation #1702
📖 Add API reference documentation generation #1702
Conversation
✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Hi @alexandrevilain. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/ok-to-test |
/retest |
Thanks for the PR |
You can check it here: https://deploy-preview-1702--kubernetes-sigs-cluster-api-openstack.netlify.app/api/v1alpha7/api |
Thank you! I will try to review this properly asap if nobody gets to it before me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks fantastic. Couple of questions inline, but I think we can merge this soon.
Thank you!
// +genclient | ||
// +genclient:Namespaced |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know why these are required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, because complete support for kubebuilder v2 is still missing in the tool ahmetb/gen-crd-api-reference-docs#15
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you hand-craft these templates, or were they copied from somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a copy-past from https://github.com/ahmetb/gen-crd-api-reference-docs/tree/master/template
docs/book/src/api/index.md
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't suppose there's any way to generate this? No worries if not, it can be hand-crafted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No idea, maybe we can create a tool to generate this after generating apis doc in the makefile
0b4f229
to
b64855a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, this is indeed very nice!
Alternative approaches
I just had a look at gen-crd-api-reference-docs and noticed the following comment:
Nowadays, I don't have a lot of time to maintain this tool. So consider using one of the above in case this repo does not work for you.
If you're an open source project, consider exposing your CRD API Reference via https://doc.crds.dev/ without much effort.
Which reminds me of https://xkcd.com/2347/ and I have, in fact, used the CRD API references on https://doc.crds.dev/ countless times as a user myself:
Those are always up-to-date and you can also browse earlier versions easily. Including a link to that website, might be a viable alternative to generating the reference documentation ourselves.
That being said, I do like the outcome, generating the reference documentation ourselves means we are not depending on an external service, and the tool appears to be well adopted within the community.
Licensing
Is there anything we need to do when copy & pasting templates from gen-crd-api-reference-docs/tree/master/template from a licensing perspective?
I see that the project does not include its own copyright notice, which makes it a bit harder to comply with the license requirement to retain the original statement.
generate-api-docs: $(GEN_CRD_API_REFERENCE_DOCS) ## Generate api documentation | ||
$(GEN_CRD_API_REFERENCE_DOCS) \ | ||
-api-dir=./api/v1alpha7 \ | ||
-config=./docs/book/gen-crd-api-reference-docs/config.json \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This files seems to be missing and I see the following error in the test results:
-template-dir=./docs/book/gen-crd-api-reference-docs/template \
-out-file=./docs/book/src/api/v1alpha6/api.md
F1005 08:16:40.577071 16819 main.go:123] failed to open config file: open ./docs/book/gen-crd-api-reference-docs/config.json: no such file or directory
make[1]: *** [Makefile:288: generate-api-docs] Error 255
make[1]: Leaving directory '/home/prow/go/src/sigs.k8s.io/cluster-api-provider-openstack'
make: *** [Makefile:255: generate] Error 2
Do we have to provide it or would the tool fall back to reasonable defaults if we don't?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh! I understand why CI was failing ... config.json
is gitignored ! Fixing this !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also happened to discover we're ignoring '*.json' just last week. We should get rid of that rule; it's weird.
/test pull-cluster-api-provider-openstack-test |
40b025d
to
694416b
Compare
/retest |
6c0f3b1
to
862ee63
Compare
Not sure what that test failure is about. I really want to get this merged, so if I get some time I'll investigate myself if nobody beats me to it. |
No idea why test are failing ... I tried rebase in case of failing CI on the |
862ee63
to
b5cf1a5
Compare
Ok it was only a bad rebase on my side. /remove-hold |
I had to rebase to make the CI pass. |
Will try to do the rebase today. |
Hello, could we add v1alpha8 please and rebase? Thanks a lot :) |
b6cac65
to
291b2ac
Compare
291b2ac
to
b550d0e
Compare
Hello @EmilienM ! Done, waiting for CI to pass. |
You can check deploy preview here: https://deploy-preview-1702--kubernetes-sigs-cluster-api-openstack.netlify.app/api/v1alpha8/api |
3066a8c
to
e80879c
Compare
I'm not sure why we have this error, let us know if you need help on this one. @mdbooth I would love to get this one at some point, not blocking anything but I'm glad @alexandrevilain can still work on it. |
e80879c
to
727e193
Compare
@EmilienM I rebased and re-run |
0fe9409
to
69baa16
Compare
Signed-off-by: alexandre.vilain <[email protected]>
69baa16
to
af5545c
Compare
@EmilienM CI is now green 👍 |
/lgtm |
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #1694
Special notes for your reviewer:
TODOs:
/hold