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

Add support for generating API group version list #172

Merged
merged 1 commit into from
Oct 12, 2020

Conversation

tengqm
Copy link
Contributor

@tengqm tengqm commented Oct 9, 2020

This has been a desired feature on kubernetes/website. (kubernetes/website#2979)

This has been a desired feature on kubernetes/website.
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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 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. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 9, 2020
@tengqm
Copy link
Contributor Author

tengqm commented Oct 9, 2020

image


fmt.Fprintf(f, h.DefaultStaticContent("API Groups"))
fmt.Fprintf(f, "<P>The API Groups and their versions are summarized in the following table.</P>")
fmt.Fprintf(f, "<TABLE class=\"col-md-8\">\n<THEAD><TR><TH>Group</TH><TH>Version</TH></TR></THEAD>\n<TBODY>\n")
Copy link
Member

Choose a reason for hiding this comment

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

Can we show what kind of format we have so that we need to print this formatting? Or probably write the expected output as comment on the function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See my comment above. It is a screenshot

@sftim
Copy link
Contributor

sftim commented Oct 9, 2020

Likely to be relevant to kubernetes/website#23809

path := filepath.Join(api.IncludesDir, fn)
f, err := os.Create(path)
defer f.Close()
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @tengqm . Nice to see different ways of presenting the information. Wondering about:
The table appears after the overview on the page?
Approximately how many unique groups (rows) are in the table? There are no links in the table and the table does not scroll or collapse.
Would it be useful to list or link to the resources found under each group (Not in the scope of this PR but may be something to think about)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are about 20 rows in the table. It is a very simple table. Listing resources under each group is doable.

Copy link
Contributor

@kbhawkey kbhawkey Oct 11, 2020

Choose a reason for hiding this comment

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

Hi @tengqm . Since this information is requested by readers, it makes sense to provide and highlight the groups
and supported versions.
Suggestion: Could this information be presented as a standalone single page or could the information be inferred
by updating the API navigation. Does it make sense to navigate by group (not category)?
I think the new information could be created, pushed/copied to the site repo, included in a md page or use a writer to create a supported docs page layout with this content.
What do you think? The changes could go in with follow-up changes.
Let's temporarily place this on hold (feel free to cancel the hold after discussion).
/hold

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Surely we can dump this data into whatever format once we have the map.
I chose to summarize this API group/version info when we are extracting definitions from the swagger spec so we don't need to traverse it again.
Since the only source of truth is the swagger spec, I'm not sure what do you mean by "could the information be inferred ...".
My immediate response (could be wrong) to your question "navigating by group" is a No. API groups (and versions) are never the first thing a user wants to check. This was true at least in the past when we have Deployment defined in both extensions group and apps group, Ingress defined in both extensions and networking group, etc. As the upstream community is gradually hardening the "core" resource types to support, the grouping of resources are much more clearer. Navigating resources by groups might be eventually something to consider.
Another fact we need to consider is that not all swagger definitions are resources, though they are also very important to users. If we put API groups on the top level, we will have to list apimachinery, meta, ... there as group names. I'm pretty sure it will drive some users crazy.
Back to your last question, splitting this information into a separate file is doable. The only concern I have is about maintenance overhead. We are already blamed by someone for the inferior quality of the generator. We have to be cautious when adding new files to manage for each release.
With that said, if we can settle down on a solution that generates 100 markdown files and copy them in batch to website, this information can be a standalone page shipped together with others.

Copy link
Contributor

Choose a reason for hiding this comment

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

Surely we can dump this data into whatever format once we have the map.
I chose to summarize this API group/version info when we are extracting definitions from the swagger spec so we don't need to traverse it again.

-- OK

Since the only source of truth is the swagger spec, I'm not sure what do you mean by "could the information be inferred ...".

-- I wonder if the table is needed if the existing information is presented differently.

My immediate response (could be wrong) to your question "navigating by group" is a No. API groups (and versions) are never the first thing a user wants to check. This was true at least in the past when we have Deployment defined in both extensions group and apps group, Ingress defined in both extensions and networking group, etc. As the upstream community is gradually hardening the "core" resource types to support, the grouping of resources are much more clearer. Navigating resources by groups might be eventually something to consider.

--OK. I just thought it would make sense since "groups" are built in categories.

Another fact we need to consider is that not all swagger definitions are resources, though they are also very important to users. If we put API groups on the top level, we will have to list apimachinery, meta, ... there as group names. I'm pretty sure it will drive some users crazy.

-- Could filter out some groups?

Back to your last question, splitting this information into a separate file is doable. The only concern I have is about maintenance overhead. We are already blamed by someone for the inferior quality of the generator. We have to be cautious when adding new files to manage for each release.
With that said, if we can settle down on a solution that generates 100 markdown files and copy them in batch to website, this information can be a standalone page shipped together with others.

-- I am thinking about Hugo modules (https://gohugo.io/hugo-modules/) instead of copying files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason I'm summarizing the API groups and versions table is that people need such information for specifying the --runtime-config flag (kubernetes/website#2979). This data can be inferred from existing page by taking note whenever you see a new API group version. In fact, that is exactly what this PR is doing. We can automatically summarize the supported API group versions into one table. If you don't know what API group version can be enabled/disabled, just check this table.
When we have this table in place, you can reference it from any place in the website.

I'm not familiar with Hugo modules.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK to add this now, though, when the big page is separated, this table could move to another page.
/lgtm
/hold cancel

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Oct 11, 2020
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 12, 2020
@k8s-ci-robot k8s-ci-robot merged commit a7cd9f6 into kubernetes-sigs:master Oct 12, 2020
@tengqm tengqm deleted the add-api-group-list branch October 13, 2020 01:06
tengqm added a commit to tengqm/website that referenced this pull request Oct 19, 2020
javidiaz pushed a commit to javidiaz/website that referenced this pull request Oct 19, 2020
dashpole pushed a commit to dashpole/website that referenced this pull request Oct 21, 2020
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.

5 participants