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

[GSoD'20] Update how the Kubernetes website serves API references #23294

Merged
merged 2 commits into from
Jan 20, 2021

Conversation

feloy
Copy link
Member

@feloy feloy commented Aug 21, 2020

This pull request implements #23809

Generating Markdown files

The goal of this PR is to generate Markdown files for the API reference, with the help of https://github.com/kubernetes-sigs/reference-docs/tree/master/gen-resourcesdocs. The result is visible at https://deploy-preview-23294--kubernetes-io-master-staging.netlify.app/docs/reference/kubernetes-api/

@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 21, 2020
@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 cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 21, 2020
@k8s-ci-robot k8s-ci-robot added language/en Issues or PRs related to English language sig/docs Categorizes an issue or PR as relevant to SIG Docs. labels Aug 21, 2020
@feloy feloy force-pushed the feloy-gsod-api-reference-ex1 branch from 9f6181b to ea5bbd1 Compare August 21, 2020 11:56
@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 Aug 21, 2020
@netlify
Copy link

netlify bot commented Aug 21, 2020

Deploy preview for kubernetes-io-master-staging ready!

Built with commit 9f6181b

https://deploy-preview-23294--kubernetes-io-master-staging.netlify.app

@netlify
Copy link

netlify bot commented Aug 21, 2020

✔️ Deploy preview for kubernetes-io-master-staging ready!

🔨 Explore the source changes: 1373ad2

🔍 Inspect the deploy logs: https://app.netlify.com/sites/kubernetes-io-master-staging/deploys/60073f18b2c43b00089a5559

😎 Browse the preview: https://deploy-preview-23294--kubernetes-io-master-staging.netlify.app

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.

BTW, you can add example shortcodes into content/en/docs/test.md.

@sftim
Copy link
Contributor

sftim commented Aug 21, 2020

/hold
do not merge

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 21, 2020
@feloy
Copy link
Member Author

feloy commented Aug 21, 2020

BTW, you can add example shortcodes into content/en/docs/test.md.

The swaggerui shortcode needs the swagger page layout (https://www.docsy.dev/docs/adding-content/shortcodes/#swaggerui)

---
title: A swagger page
type: swagger
---

I would prefer not to break the actual layout of the page.

@tengqm
Copy link
Contributor

tengqm commented Aug 21, 2020

Preview: https://deploy-preview-23294--kubernetes-io-master-staging.netlify.app/docs/reference/swagger/batch-v1/

We know about this swagger-viewer UI long before. It is simply not usable for Kubernetes APIs.

@feloy
Copy link
Member Author

feloy commented Aug 21, 2020

The idea would be to document parts of the Kubernetes API (here the batch/v1 API only), in specific places of the documentation. More on the project: https://developers.google.cn/season-of-docs/docs/participants/project-cncf-feloy

@sftim
Copy link
Contributor

sftim commented Sep 9, 2020

@feloy, is there an issue opened to track the work that this pull request is a (draft) to implement?

(if not, please open one when you have a moment)

@celestehorgan
Copy link
Contributor

@feloy Awesome to see this project underway!

A couple of general comments/questions. If you feel it's too soon to think about these @feloy, please feel free to open the issue @sftim mentioned above and record these commends over in the issue to address later.

Relevant in the scope of this PR

  1. Do we actually want to enable the Authorize/Try it out features that come default with most Swagger renderings? Kubernetes is usually deployed "on prem", i.e. by and for a specific team, rather than being an API available to anyone with auth access. Maintaining a k8s deployment for people to 'try it out' on could also pose security risks. I'm not sure this is the best option for us? What do you think, @zacharysarah?

  2. Is any k8s API truly "unversioned" (per the tag marker here: https://deploy-preview-23294--kubernetes-io-master-staging.netlify.app/docs/reference/swagger/batch-v1/), or are they the same version as the overall k8s version – in this case, v1.19?

General questions/comments better left for an issue/umbrella issue for this change:

  1. A writer-ly question for @zacharysarah. Is this...
  • The Batch API?
  • The Batch Resource(s) of the Kubernetes API?
  • The Batch Endpoint(s) of the Kubernetes API?
  • "Batch" in the Kubernetes API?
  • Other?

I'd go for either Batch API or Batch Resource of the K8s API personally.

  1. If we're going to use placeholders, i.e. /apis/batch/v1/namespaces/{namespace}/jobs/{name}, we need to:
  • make sure each section uses the same holders
  • define them centrally for people
  1. Before this goes live we need to do a sweep of the source files in https://github.com/kubernetes/api for basic language quality, i.e. capitalization at the beginning of sentences.

I think I have more but for now, this is what comes to mind.

@sftim
Copy link
Contributor

sftim commented Sep 9, 2020

BTW it's the batch API group.

@celestehorgan
Copy link
Contributor

So one major thing that came to mind/was forming as I wrote that first comment is this: based purely on the existing API Reference and @feloy's first pass at rendering the swagger output, we're going to have... UX/navigation comprehension problems between what we currently have and where we'll end up.

@feloy – please don't feel too pressured by this comment! It's a matter of SIG Docs deciding what we want and how we want to display things. Just be aware :)

For example, let's say I want to create a Job, aka POST /apis/batch/v1/namespaces/{namespace}/jobs.

In our existing API Reference, you end up here. I'll drop a screencap of the navigation to make it clearer:

Screen Shot 2020-09-09 at 11 05 02 AM

If I had to breadcrumb out this navigation structure, I'd do it like this: Kubernetes API Reference > Workload APIs > Jobs(*) > Create

(*) Mentally, as a user, I drop the "v1 batch" part of the name, because all the APIs in this section are "v1 _____" and my mind glosses over it.

If I had to do the same for the Swagger-rendered API Reference..

Screen Shot 2020-09-09 at 11 05 28 AM

I'd end up with the following: Kubernetes API Reference > Batch API > Batch v1 > POST /apis/batch/v1/namespaces/{namespace}/jobs

This brings up two things we need to think about as a group:

  • We lose the context of a group of APIs being related to "workloads"
  • Instead of "Job" being the primary resource to navigate by, now "Batch" is the primary resource we see and navigate by.

For what it's worth, I think:

  • Losing that grouping of things being "workload" related is, in an API as large as Kubernetes, problematic. The existing API reference also provides guidance documentation at the Workload API heading level which help situate the user. I think this is mitagate-able by creating congruent normal Docsy pages.
  • The same group of endpoints now being labeled "Batch" instead of "Job" is really problematic, from a UX perspective. Ideas anyone? :/

@kbhawkey
Copy link
Contributor

kbhawkey commented Sep 9, 2020

Hi. There is another doc authored by @feloy which includes some input about the project.
Is there a plan to capture these questions/ideas in a website project or issue?
The current reference is one large page with the left hand navigation grouping related resources.
I am not for or against these categories, but the categories provide a way
of modelling the content that somewhat ties into the concepts section (workloads, services, configuration and storage).
Would the new reference information be a complete reference or single pages listed in one content section?
The GVK remains an important piece.

@zacharysarah
Copy link
Contributor

To boost visibility, @feloy's project doesn't officially begin until September 13th. Please be mindful with expectations for reviews given before the project officially begins.

@feloy feloy force-pushed the feloy-gsod-api-reference-ex1 branch from 63ab0de to b0326f2 Compare September 20, 2020 08:38
@k8s-ci-robot k8s-ci-robot removed the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 20, 2020
@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 Sep 20, 2020
@feloy feloy force-pushed the feloy-gsod-api-reference-ex1 branch from 32e93e3 to add24db Compare January 19, 2021 20:05
@kbhawkey
Copy link
Contributor

Not sure why the shortcode changes were pulled out of this PR, but:
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 19, 2021
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 506f4cbc1d1223b666a0db793c9da7ddf30c139d

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 19, 2021
@zacharysarah
Copy link
Contributor

zacharysarah commented Jan 19, 2021

@sftim

let's mark the new API reference as not indexable for search engines, initially

Possibly bikeshedding, but I think disallowing in robots.txt is easier and less painful to maintain and eventually delete than adding noindex tags to individual files, given that the new reference generation method creates numerous files instead of a single page.

It won't actually de-index files from search results--folks will get "robots.txt didn't allow this" or whatever--but I think it might work temporarily.

Thoughts?

User-agent: *
Disallow: https://kubernetes.io/docs/reference/generated/kubernetes-api/*
Allow: https://kubernetes.io/docs/reference/generated/kubernetes-api/
Allow: https://kubernetes.io/docs/reference/generated/kubernetes-api/api-index/
Allow: https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.20/
Allow: https://kubernetes.io/docs/reference/kubernetes-api/labels-annotations-taints/

@sftim
Copy link
Contributor

sftim commented Jan 19, 2021

@zacharysarah that's useful bikeshedding!

@sftim
Copy link
Contributor

sftim commented Jan 19, 2021

BTW usually robot exclusion uses site-relative URLs rather than full URLs.
BTW BTW we should leave /docs/reference/generated/kubernetes-api/v1.20/ indexed I think.

@sftim
Copy link
Contributor

sftim commented Jan 20, 2021

If #26155 merges, or something similar, let's merge this.

@zacharysarah
Copy link
Contributor

zacharysarah commented Jan 20, 2021

#26155 merged, so here's an approval. @sftim, feel free to /lgtm and /hold cancel when ready. 👍🏻

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: zacharysarah

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 Jan 20, 2021
@sftim
Copy link
Contributor

sftim commented Jan 20, 2021

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 20, 2021
@sftim sftim dismissed their stale review January 20, 2021 00:19

Addressed

@sftim
Copy link
Contributor

sftim commented Jan 20, 2021

/remove-label tide/merge-method-squash
/lgtm

🎉 @feloy 🎉
(this is a huge improvement)

@k8s-ci-robot k8s-ci-robot removed the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Jan 20, 2021
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 20, 2021
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 7243fe0270c0277ba034c77e3b8bf7f4e68102e1

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. language/en Issues or PRs related to English language lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/docs Categorizes an issue or PR as relevant to SIG Docs. 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.