resourcedocs: Add Anchors to inlined definitions #186

feloy · 2020-12-11T12:09:44Z

Inlined definitions into the Markdown generated by gen-resourcesdocs are not referenceable.

These changes adds anchors to the inlined definitions.

These changes also explicitly set fragment names for Resources.

sftim

It passed CI, and the intention makes sense to me.
/lgtm

sftim · 2020-12-11T19:00:47Z

gen-resourcesdocs/templates/chapter-single-definition.tmpl

 {{.TypeDefinition | indent 2 | indent .Indent | indent .Indent}}
 {{end}}
 {{- end}}{{/* range .Fields */}}

 {{range .FieldCategories}}
-### {{.Name}}
+### {{.Name}} {#{{.Name}}}


Maybe add a comment about why the fragment identifier is set?

Do you need the set the fragment identifier if the same as the heading?
Hugo should automatically generate the anchors for the headings.

See my comment below,I would like to keep the original case of the heading (which is a Resource Name)

kbhawkey · 2020-12-11T19:22:09Z

👀

kbhawkey · 2020-12-11T19:23:47Z

gen-resourcesdocs/pkg/outputs/interface.go

@@ -28,7 +28,7 @@ type Chapter interface {
 // Section is an interface to a section of an output
 type Section interface {
 	AddContent(s string) error
-	AddTypeDefinition(s string) error
+	AddTypeDefinition(typ string, description string) error


nit: typ is the type?

Yes, as type is a reserved keyword in Go, we cannot use type as variable name

Right, typ is odd. t or something else might make it more readable.

I think this is quite common in Go, see for example https://golang.org/pkg/go/types/

Ha, interesting.

kbhawkey · 2020-12-11T19:25:40Z

gen-resourcesdocs/pkg/outputs/kwebsite/files.go

@@ -63,7 +63,6 @@ func escapeName(parts ...string) string {

 // headingID returns the ID built by hugo for a given header
 func headingID(s string) string {
-	result := strings.ToLower(s)
-	result = strings.ReplaceAll(result, " ", "-")
+	result := strings.ReplaceAll(s, " ", "-")


Is it necessary to normalize the heading?
I was under the impression that Hugo would do this.

Yes, Hugo does it by default, but puts the fragment in lowercase. I would prefer to keep the original case, as these are Resource names, and I would like to use the fragment name as text to display in the api-reference shortcode: (see https://github.com/kubernetes/website/pull/23294/files#diff-4992971bcbda91c2f2374a2e62fffda3d7852174780e027070e766c62bf87ac1 )

I need to look at the generated pages.
Could you update the shortcode to adapt the formatting of the resource name?

The shortcode has been changed in this commit: kubernetes/website@d9b18d3

The result should be visible in the paragraph https://deploy-preview-23294--kubernetes-io-master-staging.netlify.app/docs/concepts/configuration/secret/#using-imagepullsecrets (see the PodSpec API ...)

The build of the preview website fails, I can't see why

looking now. See this message:
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.

You need to rebase?

I think I made a rebase to restart the tests in the meantime. Here is the commit:
kubernetes/website@99e1b41

I also get the message, but you can see the change in the api-reference.html file of this commit

sftim · 2020-12-11T20:05:07Z

gen-resourcesdocs/templates/chapter-single-definition.tmpl

 {{.TypeDefinition | indent 2 | indent .Indent | indent .Indent}}
 {{end}}
 {{- end}}{{/* range .Fields */}}

 {{range .FieldCategories}}
-### {{.Name}}
+### {{.Name}} {#{{.Name}}}{{/* explicitly set fragment to keep capitalization */}}


👍 to the comment, thanks @feloy

kbhawkey · 2020-12-11T20:19:32Z

What happens when this code builds in CI? Are there files generated into a staging directory in this repository?
It would be great to see the generated Markdown files.
For some time now, I have wanted a way to test tooling changes by building the website with the generated output.

kbhawkey · 2020-12-14T01:48:40Z

Looking at:
https://deploy-preview-23294--kubernetes-io-master-staging.netlify.app/docs/reference/kubernetes-api/workloads-resources/pod-v1/#lifecycle

And:

"io.k8s.api.core.v1.Lifecycle": {
      "description": "Lifecycle describes actions that the management system should take in response to container lifecycle events. For the PostStart and PreStop lifecycle handlers, management of the container blocks until the action is complete, unless the container process fails, in which case the handler is aborted.",
      "properties": {
        "postStart": {
          "$ref": "#/definitions/io.k8s.api.core.v1.Handler",
          "description": "PostStart is called immediately after a container is created. If the handler fails, the container is terminated and restarted according to its restart policy. Other management of the container blocks until the hook completes. More info: https://kubernetes.io/docs/concepts/containers/container-lifecycle-hooks/#container-hooks"
        },
        "preStop": {
          "$ref": "#/definitions/io.k8s.api.core.v1.Handler",
          "description": "PreStop is called immediately before a container is terminated due to an API request or management event such as liveness/startup probe failure, preemption, resource contention, etc. The handler is not called if the container crashes or exits. The reason for termination is passed to the handler. The Pod's termination grace period countdown begins before the PreStop hooked is executed. Regardless of the outcome of the handler, the container will eventually terminate within the Pod's termination grace period. Other management of the container blocks until the hook completes or until the termination grace period is reached. More info: https://kubernetes.io/docs/concepts/containers/container-lifecycle-hooks/#container-hooks"
        }

Why is ,

 "terminationGracePeriodSeconds": {
          "description": "Optional duration in seconds the pod needs to terminate gracefully. May be decreased in delete request. Value must be non-negative integer. The value zero indicates delete immediately. If this value is nil, the default grace period will be used instead. The grace period is the duration in seconds after the processes running in the pod are sent a termination signal and the time when the processes are forcibly halted with a kill signal. Set this value longer than the expected cleanup time for your process. Defaults to 30 seconds.",
          "format": "int64",
          "type": "integer"
        },

Found under the heading Lifecycle?
How are the fields ordered on the page for PodSpec?

feloy · 2020-12-18T08:15:15Z

Looking at:
https://deploy-preview-23294--kubernetes-io-master-staging.netlify.app/docs/reference/kubernetes-api/workloads-resources/pod-v1/#lifecycle

[...]

Found under the heading Lifecycle?
How are the fields ordered on the page for PodSpec?

Hi @kbhawkey the headings are defined in the configuration file at (1)

Note that the heading Lifecyle is different from the Lifecycle Kubernetes resource.

(1) https://github.com/kubernetes-sigs/reference-docs/blob/master/gen-resourcesdocs/config/v1.20/fields.yaml#L22

feloy · 2020-12-18T08:19:09Z

What happens when this code builds in CI? Are there files generated into a staging directory in this repository?
It would be great to see the generated Markdown files.
For some time now, I have wanted a way to test tooling changes by building the website with the generated output.

The changes are not visible in this repo, but you can see the changes in the PR (1), especially in the commits (2) and (3)

(1) kubernetes/website#23294
(2) kubernetes/website@7c4fc1c
(3) kubernetes/website@dffa264

kbhawkey · 2020-12-18T14:22:33Z

Looking at:
https://deploy-preview-23294--kubernetes-io-master-staging.netlify.app/docs/reference/kubernetes-api/workloads-resources/pod-v1/#lifecycle
[...]
Found under the heading Lifecycle?
How are the fields ordered on the page for PodSpec?

Hi @kbhawkey the headings are defined in the configuration file at (1)

Note that the heading Lifecyle is different from the Lifecycle Kubernetes resource.

(1) https://github.com/kubernetes-sigs/reference-docs/blob/master/gen-resourcesdocs/config/v1.20/fields.yaml#L22

Hi @feloy
OK, this is what I thought. I found it confusing to use a Kubernetes object name as a heading if the fields under that heading do not represent the object's fields. Compare this representation to fields such as affinity, which refer/resolve to the Affinity object.
With the Affinity field, the sub-fields are in-lined below the field name but the reader does not get a hint that Affinity is an actual object (could use a link as well as inlining the object definition?).
See "$ref": "#/definitions/io.k8s.api.core.v1.Affinity",. Is there a common definition for Affinity? I see that there are common definitions for NodeAffinity, PodAffinity, and so on.
Another example is the tolerations field (array of Toleration), "$ref": "#/definitions/io.k8s.api.core.v1.Toleration".

Conversely, the initContainers and containers fields are not in-lined in PodSpec but are linked to the Container object,
initContainers ([]Container).

How did you select the text for the sub-headings? Would it help to list the fields alphabetically?
What are the headings Alpha level, Beta level, Deprecated? Does this mean that the fields under these heading are not GA?

I am reviewing this page:
https://deploy-preview-23294--kubernetes-io-master-staging.netlify.app/docs/reference/kubernetes-api/workloads-resources/pod-v1/#PodSpec

It think it would help provide a link for every field. If the fields used headings (instead of a list), then Hugo would add anchors. The generator could add anchors relative to the main headings on the page.
I noticed that some links in the text are not normalized. See the readinessGates field in PodSpec,
https://git.k8s.io/enhancements/keps/sig-network/0007-pod-ready%2B%2B.md

Should the reference separate out fields that are deprecated, such as PodSpec.serviceAccount:

 "serviceAccount": {
       "description": "DeprecatedServiceAccount is a depreciated alias for ServiceAccountName. Deprecated: Use serviceAccountName instead.",
       "type": "string"
     },

kbhawkey · 2020-12-20T20:50:24Z

Should I log an issue (to update a template) for separating the resource description (that contains a list) from the list of fields?
Possibly add a separator between the top description and the body of the resource (fields). Otherwise, the two lists blend together.

For example, StatefulSet:

StatefulSet represents a set of pods with consistent identities. Identities are defined as:

    * Network: A single stable DNS and hostname.

    * Storage: As many VolumeClaims as requested. The StatefulSet guarantees that a given network identity will always map to the same storage identity.

https://deploy-preview-23294--kubernetes-io-master-staging.netlify.app/docs/reference/kubernetes-api/workloads-resources/stateful-set-v1/#StatefulSet

feloy · 2020-12-20T21:05:14Z

Yes, please create an issue for this problem, I didn't see this problem before.

kbhawkey · 2020-12-20T21:15:16Z

Some more thoughts about intra-page links:

It would be useful to link to any field in an object or resource from another page.
Right now, the fields are list items and a heading serves as the link to the set of fields contained under a heading.
These headings are not directly related to other k8s objects/resources but instead represent configurable categories per page.

What do you think about creating links for each field/list item?
The categories help divide the page and add navigable content, but it is not clear that the names of the categories help in understanding the object hierarchy and information about the fields.

kbhawkey · 2020-12-20T21:31:46Z

Yes, please create an issue for this problem, I didn't see this problem before.

👍 See issue #188.

feloy · 2020-12-20T21:32:19Z

Some more thoughts about intra-page links:

It would be useful to link to any field in an object or resource from another page.
Right now, the fields are list items and a heading serves as the link to the set of fields contained under a heading.
These headings are not directly related to other k8s objects/resources but instead represent configurable categories per page.

What do you think about creating links for each field/list item?
The categories help divide the page and add navigable content, but it is not clear that the names of the categories help in understanding the object hierarchy and information about the fields.

Yes, I agree.
When I updated the links in the Concepts and Tasks sections to the new API reference, I realized lots of links were targeting specific fields. For the moment, I made the links target the category to which the field belongs, but it would be nice to have targetable fields.

feloy · 2020-12-29T11:07:33Z

I'm not sure, is there something to do on my side for this PR?

kbhawkey · 2021-01-05T01:57:09Z

/lgtm

kbhawkey · 2021-01-05T19:06:15Z

Thanks @feloy .
Reviewed generated files in kubernetes/website@7c4fc1c
/approve

k8s-ci-robot · 2021-01-05T19:06:25Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~OWNERS~~ [kbhawkey]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

resourcedocs: Add Anchors to inlined definitions

42c01cb

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 11, 2020

k8s-ci-robot requested review from kbhawkey and sftim December 11, 2020 12:09

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Dec 11, 2020

feloy mentioned this pull request Dec 11, 2020

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

Merged

Explicitly set anchor names

6898e87

sftim reviewed Dec 11, 2020

View reviewed changes

k8s-ci-robot assigned sftim Dec 11, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 11, 2020

kbhawkey reviewed Dec 11, 2020

View reviewed changes

Add comments for fragments

a7ec445

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 11, 2020

Fix tests

2c047b6

sftim reviewed Dec 11, 2020

View reviewed changes

k8s-ci-robot assigned kbhawkey Jan 5, 2021

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 5, 2021

k8s-ci-robot merged commit 6d86d27 into kubernetes-sigs:master Jan 5, 2021

feloy mentioned this pull request Jan 6, 2021

REQUEST: New membership for feloy kubernetes/org#2404

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resourcedocs: Add Anchors to inlined definitions #186

resourcedocs: Add Anchors to inlined definitions #186

feloy commented Dec 11, 2020 •

edited

Loading

sftim left a comment

sftim Dec 11, 2020

kbhawkey Dec 11, 2020

feloy Dec 11, 2020

kbhawkey commented Dec 11, 2020

kbhawkey Dec 11, 2020 •

edited

Loading

feloy Dec 11, 2020

kbhawkey Dec 11, 2020

feloy Dec 11, 2020

kbhawkey Dec 11, 2020

kbhawkey Dec 11, 2020

feloy Dec 11, 2020 •

edited

Loading

kbhawkey Dec 11, 2020

feloy Dec 11, 2020

kbhawkey Dec 11, 2020

kbhawkey Dec 11, 2020

feloy Dec 11, 2020

feloy Dec 11, 2020

sftim Dec 11, 2020

kbhawkey commented Dec 11, 2020 •

edited

Loading

kbhawkey commented Dec 14, 2020

feloy commented Dec 18, 2020 •

edited

Loading

feloy commented Dec 18, 2020

kbhawkey commented Dec 18, 2020 •

edited

Loading

kbhawkey commented Dec 20, 2020 •

edited

Loading

feloy commented Dec 20, 2020

kbhawkey commented Dec 20, 2020

kbhawkey commented Dec 20, 2020

feloy commented Dec 20, 2020

feloy commented Dec 29, 2020

kbhawkey commented Jan 5, 2021

kbhawkey commented Jan 5, 2021

k8s-ci-robot commented Jan 5, 2021

resourcedocs: Add Anchors to inlined definitions #186

resourcedocs: Add Anchors to inlined definitions #186

Conversation

feloy commented Dec 11, 2020 • edited Loading

sftim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbhawkey commented Dec 11, 2020

kbhawkey Dec 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feloy Dec 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbhawkey commented Dec 11, 2020 • edited Loading

kbhawkey commented Dec 14, 2020

feloy commented Dec 18, 2020 • edited Loading

feloy commented Dec 18, 2020

kbhawkey commented Dec 18, 2020 • edited Loading

kbhawkey commented Dec 20, 2020 • edited Loading

feloy commented Dec 20, 2020

kbhawkey commented Dec 20, 2020

kbhawkey commented Dec 20, 2020

feloy commented Dec 20, 2020

feloy commented Dec 29, 2020

kbhawkey commented Jan 5, 2021

kbhawkey commented Jan 5, 2021

k8s-ci-robot commented Jan 5, 2021

feloy commented Dec 11, 2020 •

edited

Loading

kbhawkey Dec 11, 2020 •

edited

Loading

feloy Dec 11, 2020 •

edited

Loading

kbhawkey commented Dec 11, 2020 •

edited

Loading

feloy commented Dec 18, 2020 •

edited

Loading

kbhawkey commented Dec 18, 2020 •

edited

Loading

kbhawkey commented Dec 20, 2020 •

edited

Loading