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

New resourcesdocs generator #179

Merged
merged 5 commits into from Dec 3, 2020

Conversation

feloy
Copy link
Contributor

@feloy feloy commented Dec 2, 2020

This generator is used to generate API Reference pages in Markdown format.

See kubernetes/website#23294 for more details

@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


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. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Dec 2, 2020
@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Dec 2, 2020
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Dec 2, 2020
Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

I'm -0 (soft opposed) to including gen-resourcesdocs/api/v1.19/swagger.json and gen-resourcesdocs/api/v1.20/swagger.json; but that's something we could clean up later.

I'm also wary of including the configuration inside gen-resourcesdocs/config/v1.20/ in the same commit as the code that consumes it. These are two related but separate changes.
LGTM, but @feloy if you were willing to split this into multiple commits then I think that works even better.

I'd like someone familiar with Golang to confirm this change is OK to merge.


kwebsite: clean
mkdir -p kwebsite/content/en/docs
go run cmd/main.go kwebsite --config-dir config/v1.20/ --file api/v1.20/swagger.json --output-dir kwebsite/content/en/docs --templates ./templates
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to take the version as a parameter somehow?

@feloy
Copy link
Contributor Author

feloy commented Dec 2, 2020

I'm -0 (soft opposed) to including gen-resourcesdocs/api/v1.19/swagger.json and gen-resourcesdocs/api/v1.20/swagger.json; but that's something we could clean up later.

It would be nice to make the build reproducible by being sure which swagger file has been used. Do you prefer we put the swagger file on the k/website repo wih the generated files?

@kbhawkey
Copy link
Contributor

kbhawkey commented Dec 2, 2020

It would be really nice if there was a job that created a temporary k8s/k8s repo, grabbed a version of the spec, copied the spec to this repo or website.
Some of the code to create this job already exists in update-imported-docs script+reference.yml (create k8s/k8s repo) which calls updateapispec target (fetch swagger.json) in reference-docs/Makefile.

Temporarily, could add a command to the update-imported-docs script to copy a swagger file or make a new script based off parts of the update script. Drop the swagger file into website.

@tengqm
Copy link
Contributor

tengqm commented Dec 3, 2020

/approve

I'm inclined to kick this in now and start the iteration.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: feloy, tengqm

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 3, 2020
@kbhawkey
Copy link
Contributor

kbhawkey commented Dec 3, 2020

I think it makes sense to get this in. I read through the code, though I have not tried to build the generator.
LGTM!!

@kbhawkey
Copy link
Contributor

kbhawkey commented Dec 3, 2020

@sftim, What do you think about getting the source code in now? The swagger files can be removed when the build targets are updated. Is the plan to publish the 1.20 pages generated from the source?

@sftim
Copy link
Contributor

sftim commented Dec 3, 2020

@kbhawkey I agree with your LGTM from #179 (comment)

With the caveat that I haven't done code review and haven't much Golang experience.
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 3, 2020
@k8s-ci-robot k8s-ci-robot merged commit 9e411d6 into kubernetes-sigs:master Dec 3, 2020
@sftim
Copy link
Contributor

sftim commented Dec 3, 2020

@feloy many congratulations 🎉🎉🎉

@feloy
Copy link
Contributor Author

feloy commented Dec 3, 2020

@feloy many congratulations 🎉🎉🎉

Thanks to everybody 🎆🎆🎆

@kbhawkey
Copy link
Contributor

kbhawkey commented Dec 3, 2020

Super!

@zacharysarah
Copy link

@feloy Congratulations! ✨ 🌟 ✨

@sftim
Copy link
Contributor

sftim commented Dec 8, 2020

I logged kubernetes/website#25505 to track switching k/website to use the new style API reference.

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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants