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

NETOBSERV-760 Add ascidoc doc #236

Merged
merged 6 commits into from
Dec 22, 2022
Merged

Conversation

jotak
Copy link
Member

@jotak jotak commented Dec 20, 2022

FTR, steps to generate:

Setup docsgen repo

  1. Clone https://github.com/jboxman-rh/openshift-apidocs-gen
  2. run npm install -g

Run it

  1. Deploy netobserv CRD on your cluster (e.g. deploy just the operator; no need to install everything, just the CRD is needed).

  2. run hack/asciidoc-gen.sh

@jotak
Copy link
Member Author

jotak commented Dec 20, 2022

I generated this file by using the same tool used in openshift-docs: https://github.com/jboxman-rh/openshift-apidocs-gen
However this is a manual process currently; it requires having a running cluster to pull openapi info from, it does not work from our static CRD definition.
We might investigate how to automate this in our builds

@jotak jotak changed the title Add ascidoc doc NETOBSERV-760 Add ascidoc doc Dec 20, 2022
@jotak
Copy link
Member Author

jotak commented Dec 20, 2022

FTR, steps to generate:

Setup docsgen repo

  1. Clone https://github.com/jboxman-rh/openshift-apidocs-gen
  2. run npm install -g

Run it

  1. Deploy netobserv CRD on your cluster (e.g. deploy just the operator; no need to install everything, just the CRD is needed).

  2. run hack/asciidoc-gen.sh

@jotak
Copy link
Member Author

jotak commented Dec 21, 2022

FTR / automation:
I'm currently running this script, that produces exactly what we want without any manual edit:

(downside: it would need to be updated if the docgen output format changes)

#!/bin/sh

jq '.definitions |= ({"io.netobserv.flows.v1alpha1.FlowCollector"})
  | del(.definitions."io.netobserv.flows.v1alpha1.FlowCollector".properties.metadata."$ref")
  | .definitions."io.netobserv.flows.v1alpha1.FlowCollector".properties.metadata += {type:"object"}
  | del(.definitions."io.netobserv.flows.v1alpha1.FlowCollector".properties.spec.properties.consolePlugin.properties.autoscaler.properties)
  | del(.definitions."io.netobserv.flows.v1alpha1.FlowCollector".properties.spec.properties.processor.properties.kafkaConsumerAutoscaler.properties)
  | .definitions."io.netobserv.flows.v1alpha1.FlowCollector".properties.spec.properties.consolePlugin.properties.autoscaler.description |= . + " Please refer to HorizontalPodAutoscaler documentation (autoscaling/v2)."
  | .definitions."io.netobserv.flows.v1alpha1.FlowCollector".properties.spec.properties.processor.properties.kafkaConsumerAutoscaler.description |= . + " Please refer to HorizontalPodAutoscaler documentation (autoscaling/v2)."' \
  openapi.json > openapi-test.json

node openshift-apidocs-gen build -c config.yaml openapi-test.json

ADOC=build/flows_netobserv_io/flowcollector-flows-netobserv-io-v1alpha1.adoc

sed -i -r 's/^:_content-type: ASSEMBLY$/:_content-type: REFERENCE/' $ADOC
sed -i -r 's/^\[id="flowcollector-flows-netobserv-io-v.+"\]$/[id="network-observability-flowcollector-api-specifications_{context}"]/' $ADOC
sed -i -r 's/= FlowCollector \[flows.netobserv.io.*/= FlowCollector API specifications/' $ADOC
sed -i -r '/^:toc: macro$/d ' $ADOC
sed -i -r '/^:toc-title:$/d ' $ADOC
sed -i -r '/^toc::\[\]$/d ' $ADOC
sed -i -r '/^== Specification$/d ' $ADOC
sed -i -r 's/^==/=/g' $ADOC

sed -i -r '/^= API endpoints/Q' $ADOC

Copy link
Contributor

@skrthomas skrthomas left a comment

Choose a reason for hiding this comment

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

A few global comments if we're to reuse for the product docs it will be helpful when copying and pasting if the content has adheres to the IBM Style Guide and Red Hat writing guidance. I ran this through our linting tool and caught all these issues. There's a few specific patterns that can be fixed maybe with search and replace. Otherwise for those not easily search/replace I've added suggestions to the PR. In the downstream docs, I've adjusted according to the following recommendations:

  • All instances of "e.g." need to be revised to "for example," or "such as,"
  • "will" needs to be "is"-this one is not as easy to find and replace because there's usually other verbs that need to be modified.
  • "may" needs to be "might"
  • "configmap" needs to be "config map"
  • "make sure" needs to be "verify"
  • Uses of "/" need to be either "or" or "and" in cases like liveness/readiness should be liveness and readiness.
  • Mention of "OpenShift" needs to be "{product-title}

@jotak
Copy link
Member Author

jotak commented Dec 22, 2022

Thanks Sara,
I'm currently reviewing the upstream CRD to make some of these changes.
The "config map" one annoys me because it makes it less explicit that we are referring to a very specific resource kind.
E.g:

certFile defines the path to the certificate file name within the config map or secret

versus

certFile defines the path to the certificate file name within the ConfigMap or Secret

I find the latter more helpful as it makes it 100% sure what we are referring about as "ConfigMap" and "Secret": these are not just natural language words.

- remove autoscaler details (this isn't "our" doc)
- remove http api details (not the purpose here)
@jotak
Copy link
Member Author

jotak commented Dec 22, 2022

@skrthomas I've updated both the upstream CRD, and the asciidoc generation script to replace OpenShift with {product-title}

Also I'm removing the "Status" part from the generated doc, it was not super interesting for users, and I see that the same is done for in-payload API doc (like here) : status field is not included. It's also more convenient because for the most part, the generated doc doesn't come from our codebase but it comes from the upstream kubernetes (including what you said about deconflict versus deescalate)

@jotak
Copy link
Member Author

jotak commented Dec 22, 2022

by the way @skrthomas / @aireilly : do you know if we can integrate the TOC on the side, like it's done for in-payload api doc? e.g. https://docs.openshift.com/container-platform/4.11/rest_api/console_apis/consoleplugin-console-openshift-io-v1alpha1.html#spec

Not necessarily for this release, if it needs investigation, but perhaps the next one ?

@jotak jotak requested a review from memodi December 22, 2022 09:41
@jotak
Copy link
Member Author

jotak commented Dec 22, 2022

I'm merging this PR but we can reopen another if there is more feedback
/approve

@openshift-ci
Copy link

openshift-ci bot commented Dec 22, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jotak

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

@openshift-merge-robot openshift-merge-robot merged commit 7f930cc into netobserv:main Dec 22, 2022
Description::
+
--
kafka describes the kafka configuration (address, topic...) to send enriched flows to.
Copy link
Contributor

@skrthomas skrthomas Jan 3, 2023

Choose a reason for hiding this comment

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

Should this be "describes the Kafka configuration..." and can we change the "(address, topic...)", so its like this: "describes the Kafka configuration, such as address or topic, to send enriched flows."

Copy link
Member Author

Choose a reason for hiding this comment

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

=> #242

(FYI, it's kind of a norm in golang's documentation to start commenting a field with it's own name, and since it's used as the source of the doc, it ends up in the asciidoc.)

KalmanMeth pushed a commit to KalmanMeth/network-observability-operator that referenced this pull request Feb 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants