Add notes to explain how API fields should be documented in conventions #1069

JoelSpeed · 2022-03-22T13:58:57Z

As an API reviewer, I would like to see thorough documentation added to new API fields so that I can understand the mechanics of the changes we are adding to our API and so that customers can understand how they can and can't use the new fields.

To encourage better documentation on ur APIs, I think we should add some notes to the conventions prompting users with a number of questions to think about when writing their godoc and some examples of how this might look in practice.

This is a first draft of this to gather initial feedback:

Is this the right location for this kind of content?
What other questions should users be asked when writing their godoc
Do we have any other recommendations about godoc that aren't already noted

ToDo:

Add more examples of where the godoc is used
Provide example of godoc output for examples to help authors understand what the user might see
Clarify any language guidance we have for writing these godoc (Eg should we follow product doc styling)
Confirm we want to split out an official API conventions/style guide

openshift-ci · 2022-03-22T13:59:01Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

mdbooth · 2022-03-22T15:04:48Z

Couple of drive-by thoughts:

Is there a generally-accepted godoc style bible? If so, is it worth linking to it and defining OpenShift policy in relation to it?
How would I annotate a deprecated field?

dhellmann

We do have some other similar advice in the conventions file, and this addition fits thematically. We also have the dev-guide directory, and so you might consider adding a separate file with API advice there if there is going to be a lot of information to document over time.

dhellmann · 2022-03-22T15:10:23Z

CONVENTIONS.md

+// + This can be used to add notes for developers that aren't intended for end users.
+type Example struct {
+  // Type allows the user to determine how to interpret the example given.
+  // It must be set to one of the following values; Documentation, Convention, or Mixed.


Is it necessary to type out the values for an enum if they're also included in the kubebuilder validation directive?

If you don't add them to the godoc, then they won't appear in oc explain as it doesn't print out the enum values. So i think the convention has been to include them in the documentation explicitly as well as having the enum for validation purposes

deads2k · 2022-03-22T16:00:47Z

Explaining this makes sense. Since so much of the openshift API is now configuration API describing how our product can be configured, detailed documentation that will automatically appear in everything consuming openAPI is extremely valuable and serves as a good starting point for writing online documentation.

JoelSpeed · 2022-03-22T17:17:28Z

Is there a generally-accepted godoc style bible? If so, is it worth linking to it and defining OpenShift policy in relation to it?

Good question, not that I'm aware of. I'm happy to expand on some of the style guidelines if we want to. I'll check in with docs as well if they have anything. One thing I was thinking is that, for example, a MachineSet is normally written as a machine set in docs for readability reasons, do we want to enforce/recommend that here too? I'm not sure, but maybe we can iterate on that

How would I annotate a deprecated field?

Not sure I know this one, I think a // DEPRECATED: <context> comment line would suffice, but perhaps @deads2k can confirm

We do have some other similar advice in the conventions file, and this addition fits thematically. We also have the dev-guide directory, and so you might consider adding a separate file with API advice there if there is going to be a lot of information to document over time.

Yeah I noticed the other notes about API conventions in here and how we differ from upstream Kube conventions, I think if we are going to put this in the dev-guide section we should move the rest of the API conventions from this doc as well.
I'm happy to do that, but would want a sign off on that from @deads2k as API conventions are his area of ownership much more than they are mine

Miciah

This looks excellent. I'm glad we're getting more of these conventions formally documented.

Miciah · 2022-03-24T04:02:40Z

dev-guide/api-conventions.md

+// + ---
+// + Note that this comment line will not end up in the generated API schema as it is
+// + preceded by a `+`. The `---` also prevents anything after it from being added to
+// + the swagger docs.
+// + This can be used to add notes for developers that aren't intended for end users.


Miciah · 2022-03-24T04:02:41Z

dev-guide/api-conventions.md

+// + This can be used to add notes for developers that aren't intended for end users.
+type Example struct {
+  // Type allows the user to determine how to interpret the example given.
+  // It must be set to one of the following values; Documentation, Convention, or Mixed.


Suggested change

// It must be set to one of the following values; Documentation, Convention, or Mixed.

// It must be set to one of the following values: Documentation, Convention, or Mixed.

Miciah · 2022-03-24T04:02:43Z

dev-guide/api-conventions.md

+type Example struct {
+  // Type allows the user to determine how to interpret the example given.
+  // It must be set to one of the following values; Documentation, Convention, or Mixed.
+  // +kubebuilder:validation:Enum:Documentation;Convention;Mixed


Suggested change

// +kubebuilder:validation:Enum:Documentation;Convention;Mixed

// +kubebuilder:validation:Enum=Documentation;Convention;Mixed

dev-guide/api-conventions.md

Miciah · 2022-03-24T04:02:46Z

dev-guide/api-conventions.md

+  // Documentation allows the user to define documentation for the example.
+  // When this value is provided, the `type` must be set to either `Documentation` or `Mixed`.
+  // The content of the documentation is free form text but must be no longer than 512 characters.
+  // +kubebuilder:validation:MaxLength:=512


I've noticed that we sometimes use := and sometimes use =, and I haven't been able to find any documentation on the distinction. As far as I can tell, they are interchangeable in practice, and if that is true, I would prefer we settle on a preferred style. We can leave that for a later PR if it requires debate.

I have erred towards := because that's how it's represented in the kubebuilder documentation. I don't think there's any different from a usage perspective though

dev-guide/api-conventions.md

Miciah · 2022-03-24T04:04:48Z

dev-guide/api-conventions.md

+  // + Note: In this example, the `platform` refers to OpenShift Container Platform and not
+  // + a cloud provider (commonly referred to among engineers as platforms).


I'm not sure this is a good example use of +. The clarification could be useful to end users reading the oc explain output or Swagger documentation, and really, the text could be written to avoid the ambiguous use of "platform" in the first place.

Perhaps I should pull this out of the Godoc in the example. The wording here is something that has been recommended to me across several API reviews so I thought it was standard wording.

Perhaps we can come up with a different wording that has less ambiguity?

CC @deads2k in case you have an opinion on this wording?

JoelSpeed · 2022-03-24T15:09:16Z

Given the feedback so far, it seems like we have a good basis here to build upon. To avoid this becoming a complete documentation through a single PR, I'm going to defer the question about marking fields deprecated for now as that seems like it deserves its own section. I will follow up to add a deprecated fields section once I've done a bit more research on that

Miciah · 2022-03-28T13:22:35Z

LGTM.

dhellmann · 2022-03-30T21:39:00Z

@JoelSpeed there are a couple of suggestions on the review. Did you want to resolve those here, or iterate?

/assign @dhellmann

JoelSpeed · 2022-03-31T10:12:00Z

@dhellmann Thanks for the prompt, missed those. I think for now let's merge this and iterate on the outstanding points. I'll try to get some opinions on the wording around "platform" and follow up to clarify that and the deprecation point

dhellmann · 2022-03-31T15:46:13Z

@JoelSpeed that works for me

/approve

openshift-ci · 2022-03-31T15:46:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dhellmann

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

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

Miciah · 2022-04-01T23:54:31Z

dev-guide/api-conventions.md

+
+  type <string>
+    Type allows the user to determine how to interpret the example given. It must be set to one of the following
+    values; Documentation, Convention, or Mixed.


Suggested change

values; Documentation, Convention, or Mixed.

values: Documentation, Convention, or Mixed.

Thanks, missed that while updating the other location, have updated now

openshift-ci · 2022-04-04T09:08:05Z

@JoelSpeed: all tests passed!

Full PR test history. Your PR dashboard.

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.

Miciah · 2022-04-04T12:43:32Z

Thanks!
/lgtm

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 22, 2022

openshift-ci bot requested review from deads2k and rvanderp3 March 22, 2022 14:01

dhellmann reviewed Mar 22, 2022

View reviewed changes

Miciah reviewed Mar 24, 2022

View reviewed changes

JoelSpeed changed the title ~~[WIP] Add notes to explain how API fields should be documented in conventions~~ Add notes to explain how API fields should be documented in conventions Mar 24, 2022

JoelSpeed marked this pull request as ready for review March 24, 2022 15:08

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 24, 2022

openshift-ci bot requested review from hardys and soltysh March 24, 2022 15:10

openshift-ci bot assigned dhellmann Mar 30, 2022

Add notes to explain how API fields should be documented in conventions

277a0d2

JoelSpeed force-pushed the godoc-conventions branch from 2539b85 to 4164fa0 Compare March 31, 2022 10:10

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 31, 2022

Miciah reviewed Apr 1, 2022

View reviewed changes

JoelSpeed added 3 commits April 4, 2022 10:01

Additional context about style guide and example of rendered output

738514d

Move API conventions out to dev-guide

75d947e

Add note about default values and wording for documenting this

09e76ff

JoelSpeed force-pushed the godoc-conventions branch from 4164fa0 to 09e76ff Compare April 4, 2022 09:01

openshift-ci bot assigned Miciah Apr 4, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 4, 2022

openshift-merge-robot merged commit e3cf697 into openshift:master Apr 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add notes to explain how API fields should be documented in conventions #1069

Add notes to explain how API fields should be documented in conventions #1069

JoelSpeed commented Mar 22, 2022 •

edited

Loading

openshift-ci bot commented Mar 22, 2022

mdbooth commented Mar 22, 2022

dhellmann left a comment

dhellmann Mar 22, 2022

JoelSpeed Mar 22, 2022

deads2k commented Mar 22, 2022

JoelSpeed commented Mar 22, 2022

Miciah left a comment

Miciah Mar 24, 2022

Miciah Mar 24, 2022

Miciah Mar 24, 2022

Miciah Mar 24, 2022

JoelSpeed Mar 24, 2022

Miciah Mar 24, 2022 •

edited

Loading

JoelSpeed Mar 24, 2022

JoelSpeed commented Mar 24, 2022

Miciah commented Mar 28, 2022

dhellmann commented Mar 30, 2022

JoelSpeed commented Mar 31, 2022

dhellmann commented Mar 31, 2022

openshift-ci bot commented Mar 31, 2022

Miciah Apr 1, 2022

JoelSpeed Apr 4, 2022

openshift-ci bot commented Apr 4, 2022

Miciah commented Apr 4, 2022

	// It must be set to one of the following values; Documentation, Convention, or Mixed.
	// It must be set to one of the following values: Documentation, Convention, or Mixed.

	// +kubebuilder:validation:Enum:Documentation;Convention;Mixed
	// +kubebuilder:validation:Enum=Documentation;Convention;Mixed

		// + Note: In this example, the `platform` refers to OpenShift Container Platform and not
		// + a cloud provider (commonly referred to among engineers as platforms).

	values; Documentation, Convention, or Mixed.
	values: Documentation, Convention, or Mixed.

Add notes to explain how API fields should be documented in conventions #1069

Add notes to explain how API fields should be documented in conventions #1069

Conversation

JoelSpeed commented Mar 22, 2022 • edited Loading

openshift-ci bot commented Mar 22, 2022

mdbooth commented Mar 22, 2022

dhellmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Mar 22, 2022

JoelSpeed commented Mar 22, 2022

Miciah 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Miciah Mar 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoelSpeed commented Mar 24, 2022

Miciah commented Mar 28, 2022

dhellmann commented Mar 30, 2022

JoelSpeed commented Mar 31, 2022

dhellmann commented Mar 31, 2022

openshift-ci bot commented Mar 31, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Apr 4, 2022

Miciah commented Apr 4, 2022

JoelSpeed commented Mar 22, 2022 •

edited

Loading

Miciah Mar 24, 2022 •

edited

Loading