gen-apidocs: include create-only resources in the generated ToC - #473
abhinav-phi wants to merge 2 commits into
Conversation
IsTopLevelResource only recognized resources with a List operation, so group-based ToC generation (auto-detect) skipped resources that only support Create. This dropped the authentication.k8s.io group (TokenRequest, TokenReview, SelfSubjectReview) and the authorization.k8s.io review types (SubjectAccessReview and friends) from the generated navigation: their group directories and category pages were never emitted, even though the group-versions page lists them. Treat a top-level Create operation as evidence of a top-level resource as well. For TokenRequest this also required attaching the Write Operations category to the definition in the special case that maps it to the createCoreV1NamespacedServiceAccountToken operation (the category was built and the operation linked, but never appended to OperationCategories, so the operation type stayed empty and the definition still failed the check). With --auto-detect --backend=hugo-md on the v1.36 spec this emits authentication/ (token-request-v1, token-review-v1, self-subject-review-v1) and authorization/ (subject-access-review-v1, local-subject-access-review-v1, self-subject-access-review-v1, self-subject-rules-review-v1), matching the pages published up to the v1.35 docs. Signed-off-by: abhinav-phi <alpha9coder@gmail.com>
kubernetes-prow
Bot
added
the
cncf-cla: yes
|
Welcome @abhinav-phi! |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: abhinav-phi The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
kubernetes-prow
Bot
added
the
size/S
Caesarsage
left a comment
Caesarsage
left a comment
There was a problem hiding this comment.
Nice find, the dropped OperationCategory is easy to miss. I reproduced your result on the v1.37 spec: both groups come back, 162 pages either way.
Have you copied the output into k/website and built it locally?
Mine failed with 18 REF_NOT_FOUND errors. Three of the admission-policy pages relref the definitions/ pages you're removing, so the website change needs to go in at the same time as this one.
While you're there, look at the page you just created, authentication/token-request-v1.md. Its request body and all three responses link to the CSI driver page instead of to itself. There are two different TokenRequest kinds, authentication.v1 and storage.v1, and the link map in markdown.go is keyed on the kind name alone, so one overwrites the other. Same thing makes Pod's eviction op point at the wrong Eviction today. So this needs a fix in markdown.go as well. That can be a follow-up PR, but please hold this one until it lands.
Last thing: the golden tests pass on your base. That golden came in with #460, which you're already on, so that note in the description can go.
Left a few smaller things inline.
| // API resource (one that has its own List endpoint), as opposed to a | ||
| // subresource (Scale, Eviction) or utility type (Status, WatchEvent). | ||
| // Some resources, such as TokenRequest and the SubjectAccessReview family, | ||
| // only support Create; they are top-level resources too. |
There was a problem hiding this comment.
The first sentence is now the opposite of what the function does, and Binding (a pods/binding subresource) gets promoted by this change, so the "as opposed to a subresource" part doesn't hold either.
| // only support Create; they are top-level resources too. | |
| // IsTopLevelResource reports whether this definition is a top-level API | |
| // resource, as opposed to a utility type (Status, WatchEvent) or a field type | |
| // that never appears on the wire by itself. | |
| // | |
| // A List endpoint is the usual marker, but create-only resources such as | |
| // TokenRequest and the SubjectAccessReview family qualify too. |
| for _, c := range d.OperationCategories { | ||
| if c == nil || c.Name != "Write Operations" { | ||
| continue | ||
| } | ||
| for _, op := range c.Operations { | ||
| if op == nil { | ||
| continue | ||
| } | ||
| if op.Type.Name == "Create" { | ||
| return true | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
| for _, c := range d.OperationCategories { | |
| if c == nil || c.Name != "Write Operations" { | |
| continue | |
| } | |
| for _, op := range c.Operations { | |
| if op == nil { | |
| continue | |
| } | |
| if op.Type.Name == "Create" { | |
| return true | |
| } | |
| } | |
| } |
There is no need for this extra loop.
This can be one pass instead of walking OperationCategories twice, same conditions, same result:
for _, c := range d.OperationCategories {
if c == nil {
continue
}
for _, op := range c.Operations {
if op == nil {
continue
}
switch c.Name {
case "Read Operations":
if op.Type.Name == "List" || op.Type.Name == "List All Namespaces" {
return true
}
case "Write Operations":
if op.Type.Name == "Create" {
return true
}
}
}
}
return false
| @@ -831,10 +831,14 @@ func (c *Config) mapOperationsToDefinitions() error { | |||
|
|
|||
| o.Definition = d | |||
| o.Definition.InToc = true | |||
There was a problem hiding this comment.
Now that the category is attached and buildGroupBasedCategories marks the definition, this line is redundant and it's the other half of what broke TokenRequest in the first place: it told writeDefinitions to skip the page while the missing category kept it out of the ToC, so it fell through both. Worth dropping so the fix reads on its own.
There was a problem hiding this comment.
nit: Since you're here, this XXX: reads like a bug marker but it's just describing the operation ID. Something like "TokenRequest is created through a ServiceAccount subresource, so its operation ID doesn't follow the ${group}${version}${resource} pattern the generic matcher below uses" would save the next person a detour
- IsTopLevelResource: single pass over OperationCategories with a switch, and a doc comment that no longer claims List-only resources are excluded - TokenRequest special case: drop the redundant o.Definition.InToc line; attaching the Write Operations category is what places the definition - Reword the XXX comment to explain why TokenRequest's operation ID does not match the generic pattern Signed-off-by: abhinav-phi <alpha9coder@gmail.com>
kubernetes-prow
Bot
added
size/M
|
Thanks @Caesarsage for the quick review and for reproducing it on the v1.37 spec — all four suggestions are applied in 9cecf0f:
To answer your question: I did not copy the output into k/website and build it there. Two reasons:
|
|
@Caesarsage all four of your suggestions are applied in 9cecf0f (single-pass Could I ask for a re-review when you have a moment? Happy to iterate on anything else. |
2 participants
Problem
With
--auto-detect,IsTopLevelResourceonly recognized definitions that have a List operation ("Read Operations" category,List/List All Namespaces). Resources that only support Create were therefore never classified as top-level, andbuildGroupBasedCategoriesdropped them entirely — their API group's category/directory and per-resource pages were never emitted.This silently dropped two whole groups from the generated navigation (kubernetes/website#57313):
authentication.k8s.io: TokenRequest, TokenReview, SelfSubjectReviewauthorization.k8s.ioreview types: SubjectAccessReview, LocalSubjectAccessReview, SelfSubjectAccessReview, SelfSubjectRulesReviewBoth groups are listed on the generated
group-versions.mdpage but had no pages. Up to the v1.35 docs these pages existed (e.g./docs/reference/kubernetes-api/authentication-resources/token-request-v1/); the migration to the markdown backend removed them because the generator no longer produced them. Verified against the v1.36 spec: baseline run produces noauthentication/orauthorization/directory.Fix
IsTopLevelResourcealso accepts a top-level Create operation ("Write Operations" category,Createtype) as evidence of a top-level resource.TokenRequestspecial case (its operation iscreateCoreV1NamespacedServiceAccountToken, mapped inmapOperationsToDefinitions), the category was constructed and the operation linked, but the category was never appended tod.OperationCategories, ando.Typewas never set — so even with fix 1 alone TokenRequest would still fail the check. Attach the category (with the typed operation) to the definition.Result
With
--auto-detect --backend=hugo-mdon the v1.36 spec:authentication/:_index.md,token-request-v1.md,token-review-v1.md,self-subject-review-v1.mdauthorization/:_index.md,subject-access-review-v1.md,local-subject-access-review-v1.md,self-subject-access-review-v1.md,self-subject-rules-review-v1.mdmatching the per-group structure of the other groups. The group
_index/weights of existing groups shift by two steps (Authentication and Authorization now occupy their alphabetical slots); all page content of existing resources is unchanged.Notes
TestWriteOperationGolden/TestWriteOperationGoldenHugoModefail on master at this commit (checked without this change) because the checked-inoperation-list-hugo.golden.mdpredates the pipe-table formatting of7e8927c; the failures are unrelated to this diff (identical failures before/after). Happy to refresh goldens in a separate PR.make apimd-hugo && copyapimdrestores the missing group directories. The new resource pages forauthentication/authorizationland incontent/en/docs/reference/kubernetes-api/authentication/and.../authorization/.