Update API type documentation for 'certmanager', 'meta' and 'acme' API groups #3031

munnerz · 2020-06-24T11:13:07Z

What this PR does / why we need it:

I've gone through these two API groups and added/updated documentation for all fields. I still need to go through the acme API group and apply similar changes.

This also includes a const being renamed (i.e. not changing the value of it), which I have separated into its own commit to make it easier to review.

Release note:

Improve documentation of API types displayed via `kubectl explain`

/cc @meyskens @JoshVanL

deploy/crds/crd-certificaterequests.yaml

deploy/crds/crd-clusterissuers.yaml

meyskens · 2020-06-24T12:34:30Z

@munnerz looking great! just these few nits

wallrj

I've gone through the certificate and certificaterequest docs....but just realised that they are the v1alpha2 docs. All those changes also apply to the v1alpha3 docs too...but my question is why bother updating the v1alpha2 docs at all?

pkg/apis/certmanager/v1alpha2/types.go

pkg/apis/certmanager/v1alpha2/types_certificaterequest.go

munnerz · 2020-06-26T08:20:42Z

All those changes also apply to the v1alpha3 docs too

This PR updates both v1alpha2 and v1alpha3 😄

We document both because both are valid API versions that you can use to talk to the apiserver with. To not document one would mean that some set of users (those consuming older api versions) have no documentation. For the most part, these comments are identical. But there are a few specific areas where they are not (where we have changed behaviours 😄 )

wallrj

I've gone through the v1alpha3 issuer types and noted a few typos and inconsistencies.

Generally:

Should each docstring start with the name of the documented field?
Personally I think it is laborious and looks clumsy when reading the source code, but I know that some of the golang linters enforce this rule...is it because it is used by the godoc tool to match comments to fields? If so, perhaps we should just automatically enforce this through a linting tool.
Use of full stops. Most doc strings have them, some don't. Can we use a linting tool to enforce this too?
Use Certificate vs certificate when talking about Certificate custom resources vs x509 certificate.

pkg/apis/acme/v1alpha3/types_issuer.go

wallrj · 2020-06-26T08:10:31Z

pkg/apis/acme/v1alpha3/types_issuer.go

 	PrivateKey cmmeta.SecretKeySelector `json:"privateKeySecretRef"`

 	// Solvers is a list of challenge solvers that will be used to solve
 	// ACME challenges for the matching domains.
+	// Solver configurations must be provided in order to obtain certificates
+	// from an ACME server.
+	// For more information, see: https://cert-manager.io/docs/configuration/acme/


Maybe this would be a better place to explain that solvers are used in order of DNS specificity.
Currently this is explained below on the ACMEChallengeSolver > Selector field

wallrj · 2020-06-26T08:12:00Z

pkg/apis/acme/v1alpha3/types_issuer.go

 type ACMEChallengeSolver struct {
 	// Selector selects a set of DNSNames on the Certificate resource that
 	// should be solved using this challenge solver.
+	// If not specified, the solver will be treated as the 'default' solver
+	// with the lowest priority, i.e. if any other solver has a more specific
+	// match, it will be used instead.


This took me a while to understand this. I think it would be worth describe this solver priority on the ACMEIssuer > Solvers field as well or instead.

pkg/apis/acme/v1alpha3/types_issuer.go

wallrj

A few more comments and suggestions.

I only reviewed the v1alpha3 docs, not the internal, or v1alpha2 docs....on the basis that the suggestions apply to those versions too (where applicable).

It's great that you've added all this extra detail. Thanks.

pkg/apis/acme/v1alpha3/types_order.go

pkg/apis/certmanager/v1alpha3/types.go

pkg/apis/certmanager/v1alpha3/types_issuer.go

pkg/apis/meta/v1/types.go

wallrj · 2020-06-26T08:58:46Z

pkg/internal/apis/acme/types_issuer.go

+// ACMEIssuer contains the specification for an ACME issuer.
+// This uses the RFC8555 specification to obtain certificates by completing
+// 'challenges' to prove ownership of domain identifiers.
+// Earlier draft versions of the ACME specification are not supported.


Do we really need to document the internal types too? Do these ever get published? It seems like we need a tool to automate the syncing of these doc strings.

They don't get published, but I opted to do this a) for consistency with other API doc packages, b) because it's not that difficult to copy and paste them over and c) for consistency with upstream APIs 😄

+1 to something to automate syncing, although there are slight tweaks made (e.g. removing the kubebuilder annotations and the optional markers etc)

Signed-off-by: James Munnelly <james@munnelly.eu>

…ationKey Signed-off-by: James Munnelly <james@munnelly.eu>

Signed-off-by: James Munnelly <james@munnelly.eu>

wallrj

Thanks @munnerz

/lgtm

jetstack-bot · 2020-06-26T14:05:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: munnerz, wallrj

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~~ [munnerz]

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

munnerz · 2020-06-26T15:06:51Z

/kind cleanup

jetstack-bot requested review from JoshVanL and meyskens June 24, 2020 11:13

munnerz force-pushed the update-api-doc-comments branch from 1f00938 to 1ac7bb5 Compare June 24, 2020 11:32

meyskens reviewed Jun 24, 2020

View reviewed changes

deploy/crds/crd-certificaterequests.yaml Outdated Show resolved Hide resolved

meyskens reviewed Jun 24, 2020

View reviewed changes

deploy/crds/crd-certificaterequests.yaml Outdated Show resolved Hide resolved

meyskens reviewed Jun 24, 2020

View reviewed changes

deploy/crds/crd-clusterissuers.yaml Outdated Show resolved Hide resolved

meyskens reviewed Jun 24, 2020

View reviewed changes

deploy/crds/crd-clusterissuers.yaml Outdated Show resolved Hide resolved

munnerz added this to the v0.16 milestone Jun 25, 2020

jetstack-bot added dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. and removed dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. labels Jun 25, 2020

wallrj requested changes Jun 26, 2020

View reviewed changes

jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 26, 2020

munnerz force-pushed the update-api-doc-comments branch from 3478c1f to 5eadb4c Compare June 26, 2020 09:28

jetstack-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 26, 2020

munnerz force-pushed the update-api-doc-comments branch from 5eadb4c to 82d4da1 Compare June 26, 2020 09:33

jetstack-bot added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. and removed dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. labels Jun 26, 2020

munnerz force-pushed the update-api-doc-comments branch from 82d4da1 to 1fbb08d Compare June 26, 2020 09:43

jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 26, 2020

munnerz added 11 commits June 26, 2020 14:47

Update API type documentation for 'certmanager' and 'meta' API groups

aacaaa2

Signed-off-by: James Munnelly <james@munnelly.eu>

Update generated files

a818975

Signed-off-by: James Munnelly <james@munnelly.eu>

Rename CRPrivateKeyAnnotationKey -> CertificateRequestPrivateKeyAnnot…

6caa4c4

…ationKey Signed-off-by: James Munnelly <james@munnelly.eu>

Address review feedback

463e08a

Signed-off-by: James Munnelly <james@munnelly.eu>

generated files

0fbb31b

Signed-off-by: James Munnelly <james@munnelly.eu>

Update acme API group doc comments

b8d1f29

Signed-off-by: James Munnelly <james@munnelly.eu>

generated files

565210f

Signed-off-by: James Munnelly <james@munnelly.eu>

address review feedback

8f0b814

Signed-off-by: James Munnelly <james@munnelly.eu>

generated files

d01d921

Signed-off-by: James Munnelly <james@munnelly.eu>

review feedback

bfe3dd2

Signed-off-by: James Munnelly <james@munnelly.eu>

generated files

7d1ba9e

Signed-off-by: James Munnelly <james@munnelly.eu>

munnerz force-pushed the update-api-doc-comments branch from 0b66d64 to ca29def Compare June 26, 2020 13:50

jetstack-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 26, 2020

Fix up use of omitempty and optional in Cloudflare config struct

636083f

Signed-off-by: James Munnelly <james@munnelly.eu>

munnerz force-pushed the update-api-doc-comments branch from ca29def to 636083f Compare June 26, 2020 13:51

munnerz changed the title ~~Update API type documentation for 'certmanager' and 'meta' API groups~~ Update API type documentation for 'certmanager', 'meta' and 'acme' API groups Jun 26, 2020

wallrj approved these changes Jun 26, 2020

View reviewed changes

jetstack-bot assigned wallrj Jun 26, 2020

jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 26, 2020

jetstack-bot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Jun 26, 2020

jetstack-bot merged commit 9dc044d into cert-manager:master Jun 26, 2020

munnerz deleted the update-api-doc-comments branch June 26, 2020 15:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update API type documentation for 'certmanager', 'meta' and 'acme' API groups #3031

Update API type documentation for 'certmanager', 'meta' and 'acme' API groups #3031

munnerz commented Jun 24, 2020

meyskens commented Jun 24, 2020

wallrj left a comment

munnerz commented Jun 26, 2020

wallrj left a comment

wallrj Jun 26, 2020

wallrj Jun 26, 2020

wallrj left a comment

wallrj Jun 26, 2020

munnerz Jun 26, 2020

wallrj left a comment

jetstack-bot commented Jun 26, 2020

munnerz commented Jun 26, 2020

Update API type documentation for 'certmanager', 'meta' and 'acme' API groups #3031

Update API type documentation for 'certmanager', 'meta' and 'acme' API groups #3031

Conversation

munnerz commented Jun 24, 2020

meyskens commented Jun 24, 2020

wallrj left a comment

Choose a reason for hiding this comment

munnerz commented Jun 26, 2020

wallrj left a comment

Choose a reason for hiding this comment

wallrj Jun 26, 2020

Choose a reason for hiding this comment

wallrj Jun 26, 2020

Choose a reason for hiding this comment

wallrj left a comment

Choose a reason for hiding this comment

wallrj Jun 26, 2020

Choose a reason for hiding this comment

munnerz Jun 26, 2020

Choose a reason for hiding this comment

wallrj left a comment

Choose a reason for hiding this comment

jetstack-bot commented Jun 26, 2020

munnerz commented Jun 26, 2020