Implement best-practice Source documentation #201

evankanderson · 2021-01-15T23:50:15Z

See https://docs.google.com/document/d/1UKyjA0nfNyvDIiF86YEeskV5_pZ_kP07AoDHrDUq8Xo/edit#heading=h.los3tdhhh2fb for the general template, though I took a few liberties as I was actually writing it up.

Changes

Adds end-user documentation for the RabbitMQ Source.

📖 End user documentation
📖 Moved contributor documentation to DEVELOPMENT.md

/kind documentation

Release Note

Added user-facing documentation for the RabbitMQ source

Docs

Will update the sources docs to point to this page once merged.

codecov · 2021-01-15T23:55:22Z

Codecov Report

Merging #201 (25cc6a2) into master (abdfa85) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #201   +/-   ##
=======================================
  Coverage   72.01%   72.01%           
=======================================
  Files          28       28           
  Lines        1451     1451           
=======================================
  Hits         1045     1045           
  Misses        324      324           
  Partials       82       82

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update abdfa85...25cc6a2. Read the comment docs.

evankanderson · 2021-01-16T05:20:21Z

/assign @abrennan89 @n3wscott @lionelvillard

/hold

Want to give a chance to review and see if this provides the documentation level we expect before submitting.

evankanderson · 2021-01-16T05:20:59Z

source/README.adoc

+
+| `type` | `dev.knative.rabbitmq.event` |
+| `source` | `/apis/v1/namespace/*$NS*/rabbitmqsources/*$NAME*#*$TOPIC*`
+   | `NS`, `NAME` and `TOPIC` are derived from the Source configuration.


One neat thing about Asciidoc is that I can split a table across multiple lines without having to pull out the <tr> tags.

evankanderson · 2021-01-16T05:21:39Z

source/README.adoc

+kind: RabbitmqSource
+metadata:
+  name: rabbitmq-source
+spec:


Note that I omit fields from the sample deliberately, to show what sort of arguments are required vs optional.

I'd maybe put something like <optional> or add call outs for optional fields?

Done, not sure if that's what you were thinking of.

vaikas

Thanks!!

vaikas · 2021-01-16T08:51:36Z

source/README.adoc

+you can use
+https://www.rabbitmq.com/kubernetes/operator/operator-overview.html[the RabbitMQ
+operator] to set up a RabbitMQ installation. An understanding of RabbitMQ
+concepts like Brokers, Exchanges, and Queues will alos be helpful.


nit s/alos/also/

vaikas · 2021-01-18T11:27:08Z

source/README.adoc

+
+=== Samples
+
+(in CloudEvents JSON format):


Since there's if/else in the payload (for example, subject is empty string if no message ID is present), I wonder if we should also include a RabbitMQ message as input and then showing how it gets transformed into a CloudEvent.

Interestingly, it seems like the ability to publish a rabbitmq event from the command line is harder than I'd expect. I found a rabbitmqadmin here that can do it: https://www.rabbitmq.com/management-cli.html

abrennan89 · 2021-01-18T13:28:10Z

source/README.adoc

+The RabbitMQ source translates messages on a RabbitMQ exchange to CloudEvents
+over HTTP for use with Knative Eventing. The Source can bind to an existing
+exchange or create a new one, as needed.


Suggested change

The RabbitMQ source translates messages on a RabbitMQ exchange to CloudEvents

over HTTP for use with Knative Eventing. The Source can bind to an existing

exchange or create a new one, as needed.

The RabbitMQ source translates messages from a RabbitMQ exchange to CloudEvents, which can then be used with Knative Eventing over HTTP.

The source can bind to an existing RabbitMQ exchange, or create a new exchange if required.

(This is suggesting sentence-per-line?)

If you look at it it's actually suggestions for slightly different wording and to make it more consistent, but yeah I'd format this as suggested to make it more readable. Up to you though.

abrennan89 · 2021-01-18T13:28:46Z

source/README.adoc

+
+== Published Events
+
+All messages received by the Source will be published with the following schema:


Suggested change

All messages received by the Source will be published with the following schema:

All messages received by the source are published with the following schema:

Use lowercase consistently for source IMHO

Done (lowercasing)

abrennan89 · 2021-01-18T13:30:10Z

source/README.adoc

+
+All messages received by the Source will be published with the following schema:
+
+.Table Event Attributes


Suggested change

.Table Event Attributes

.Event attributes

Ha! I mis-read the asciidoc cheat-sheet. Thanks!

abrennan89 · 2021-01-18T13:31:14Z

source/README.adoc

+| `type` | `dev.knative.rabbitmq.event` |
+| `source` | `/apis/v1/namespace/*$NS*/rabbitmqsources/*$NAME*#*$TOPIC*`
+   | `NS`, `NAME` and `TOPIC` are derived from the Source configuration.
+| `id` | a unique id | This uses the MessageId if available, and a UUID otherwise.


abrennan89 · 2021-01-18T13:31:54Z

source/README.adoc

+| `source` | `/apis/v1/namespace/*$NS*/rabbitmqsources/*$NAME*#*$TOPIC*`
+   | `NS`, `NAME` and `TOPIC` are derived from the Source configuration.
+| `id` | a unique id | This uses the MessageId if available, and a UUID otherwise.
+| `subject` | the message's id | empty string if no message ID is present.


abrennan89 · 2021-01-18T13:32:17Z

source/README.adoc

+| `id` | a unique id | This uses the MessageId if available, and a UUID otherwise.
+| `subject` | the message's id | empty string if no message ID is present.
+| `datacontenttype` | `application/json` | Currently static.
+| `key` | the message's id | empty string if no message ID is present.


abrennan89 · 2021-01-18T13:33:35Z

source/README.adoc

+| `key` | the message's id | empty string if no message ID is present.
+|===
+
+The event's payload will be set to the data content of the message.


Suggested change

The event's payload will be set to the data content of the message.

The payload of the event is set to the data content of the message.

Use present tense instead of future consistently, and avoid possessives, e.g. "event's" where possible.

abrennan89 · 2021-01-18T13:33:57Z

source/README.adoc

+toc::[]
+
+
+== Published Events


Suggested change

== Published Events

== Published events

Shouldn't this be title case?

abrennan89 · 2021-01-18T13:34:58Z

source/README.adoc

+
+=== Samples
+
+(in CloudEvents JSON format):


Avoid unnecessary parenthesis, suggest:
.CloudEvent JSON format

abrennan89 · 2021-01-18T13:35:37Z

source/README.adoc

+}
+----
+
+== Creating and Managing Sources


Suggested change

== Creating and Managing Sources

== Creating and managing sources

Sentence case headings

abrennan89 · 2021-01-18T13:35:52Z

source/README.adoc

+----
+
+== Creating and Managing Sources
+


Seems like there's an extra line here

There's a thin rule after each H2 section -- you can see the same thing at "Administration" and "Published events". Note that both of those have text immediately following, rather than a table.

abrennan89 · 2021-01-18T13:43:57Z

source/README.adoc

+Note that not all parameters need to be specified; unspecified parameters will be
+defaulted false / empty.


Suggested change

Note that not all parameters need to be specified; unspecified parameters will be

defaulted false / empty.

Note that not all parameters must be specified.

Unspecified optional parameters default to `false` or empty.

I rewrote this a little more:

Note that many parameters do not need to be specified. Unspecified optional
parameters will be defaulted to false / or "" (empty string).

abrennan89 · 2021-01-18T13:47:47Z

source/README.adoc

+      name: event-display
+----
+
+== Administration


Not sure I like this in the one file, I think the idea was more that there would be an introduction "guide" with conceptual info etc and then an Admin guide with all of this info as a separate page. Also, I would avoid having any heading that is directly followed by another heading with nothing between; there should be some paragraph or something here explaining what this admin section is

abrennan89 · 2021-01-18T13:50:03Z

source/README.adoc

+You will need a https://www.rabbitmq.com/[RabbitMQ] installation. On Kubernetes,
+you can use
+https://www.rabbitmq.com/kubernetes/operator/operator-overview.html[the RabbitMQ
+operator] to set up a RabbitMQ installation. An understanding of RabbitMQ
+concepts like Brokers, Exchanges, and Queues will alos be helpful.


Suggested change

You will need a https://www.rabbitmq.com/[RabbitMQ] installation. On Kubernetes,

you can use

https://www.rabbitmq.com/kubernetes/operator/operator-overview.html[the RabbitMQ

operator] to set up a RabbitMQ installation. An understanding of RabbitMQ

concepts like Brokers, Exchanges, and Queues will alos be helpful.

* A https://www.rabbitmq.com/[RabbitMQ] installation. On Kubernetes, you can use https://www.rabbitmq.com/kubernetes/operator/operator-overview.html[the RabbitMQ

operator] to set up a RabbitMQ installation.

* An understanding of RabbitMQ concepts like Brokers, Exchanges, and Queues.

Heading prerequisites negates need for bits like "you will need", and the "helpful" bit etc should be removed - something's either needed and should be listed here or isn't IMHO

abrennan89 · 2021-01-18T13:50:49Z

source/README.adoc

+Install from the nightly build:
+
+[source,sh]
+----
+kubectl apply -f https://storage.googleapis.com/knative-nightly/eventing-rabbitmq/latest/rabbitmq-source.yaml
+----


Suggested change

Install from the nightly build:

[source,sh]

----

kubectl apply -f https://storage.googleapis.com/knative-nightly/eventing-rabbitmq/latest/rabbitmq-source.yaml

----

* Install the source from the nightly build:

[source,sh]

----

kubectl apply -f https://storage.googleapis.com/knative-nightly/eventing-rabbitmq/latest/rabbitmq-source.yaml

----

abrennan89 · 2021-01-18T13:52:00Z

source/README.adoc

+The standard `config-observability`, `config-logging` et al ConfigMaps may be
+used to manage the logging and metrics configuration.


No et al please :)

Suggested change

The standard `config-observability`, `config-logging` et al ConfigMaps may be

used to manage the logging and metrics configuration.

You can use the standard `config-observability` and `config-logging` ConfigMaps to manage the logging and metrics configuration.

Some samples or links here?

I'm not sure we have good ones right now, let me link the bad ones (the configmaps in the Eventing repo).

abrennan89

So far looks pretty good, added some minor suggestions. Not 100% happy with the layout re headings etc but we can revisit as part of evolving the template.

evankanderson · 2021-01-26T22:50:57Z

Updated.

You can preview here:

https://github.com/evankanderson/eventing-rabbitmq/tree/gold-star/source

abrennan89

/lgtm

abrennan89 · 2021-01-27T19:38:49Z

source/README.adoc

+
+== Published events
+
+All messages received by the source are published with the following schema:


Suggested change

All messages received by the source are published with the following schema:

All messages received by the RabbitMQ source are published with the following schema:

abrennan89 · 2021-01-27T19:39:43Z

source/README.adoc

+The RabbitMQ source translates messages on a RabbitMQ exchange to CloudEvents,
+which can then be used with Knative Eventing over HTTP. The source can bind to
+an existing RabbitMQ exchange, or create a new exchange if required.


Suggested change

The RabbitMQ source translates messages on a RabbitMQ exchange to CloudEvents,

which can then be used with Knative Eventing over HTTP. The source can bind to

an existing RabbitMQ exchange, or create a new exchange if required.

The RabbitMQ source translates messages on a RabbitMQ exchange to CloudEvents,

which can then be used with Knative Eventing over HTTP. The RabbitMQ source can bind to

an existing RabbitMQ exchange, or create a new exchange if required.

Can we clarify what's meant here by "can then be used with Knative Eventing over HTTP" - used with how?

Added more detail linking to the eventing Addressable concept.

abrennan89 · 2021-01-27T19:48:51Z

source/README.adoc

+.Event attributes
+|===
+| Attribute | Value | Notes
+
+| `type` | `dev.knative.rabbitmq.event` |
+| `source` | `/apis/v1/namespace/*$NS*/rabbitmqsources/*$NAME*#*$TOPIC*`
+   | `NS`, `NAME` and `TOPIC` are derived from the source configuration
+| `id` | A unique ID | This uses the `MessageId` if available, and a UUID otherwise
+| `subject` | The ID of the message | Empty string if no message ID is present
+| `datacontenttype` | `application/json` | Currently static
+| `key` | The ID of the message | Empty string if no message ID is present
+|===
+
+The payload of the event is set to the data content of the message.


Suggested change

.Event attributes

|===

| Attribute | Value | Notes

| `type` | `dev.knative.rabbitmq.event` |

| `source` | `/apis/v1/namespace/*$NS*/rabbitmqsources/*$NAME*#*$TOPIC*`

| `NS`, `NAME` and `TOPIC` are derived from the source configuration

| `id` | A unique ID | This uses the `MessageId` if available, and a UUID otherwise

| `subject` | The ID of the message | Empty string if no message ID is present

| `datacontenttype` | `application/json` | Currently static

| `key` | The ID of the message | Empty string if no message ID is present

|===

The payload of the event is set to the data content of the message.

[source,json]

----

{

"specversion": "1.0",

"type": "dev.knative.rabbitmq.event",

"source": "/apis/v1/namespaces/<namespace>/rabbitmqsources/<name>#<topic>", <1>

"id": "<unique_ID>", <2>

"time": "<timestamp>",

"subject": "", <3>

"datacontenttype": "application/json",

"key": "", <4>

"data": "Hello rabbitmq!" <5>

}

----

<1> `<namespace>`, `<name>` and `<topic>` are derived from the source configuration.

<2> Uses the `MessageId` if available, and a UUID otherwise.

<3> The ID of the message, or an empty string if no message ID is present.

<4> The ID of the message, or an empty string if no message ID is present.

<5> The payload of the event is set to the data content of the message.

Rather than the table, I'd suggest showing the actual schema as an example like this, where there are callouts describing any variables

So no table at all? I have the actual schema in the example, though with values filled in.

abrennan89 · 2021-01-27T19:50:42Z

source/README.adoc

+
+The payload of the event is set to the data content of the message.
+
+=== Samples


Suggested change

=== Samples

=== Example

Let's maybe change this in the template to example rather than samples

RabbitMQ only has one type of event, but that might not be true of all sources. Do we still want an example in addition to the sample schema format instead of event attributes?

abrennan89 · 2021-01-27T19:51:26Z

source/README.adoc

+For a message published with the payload "Hello rabbitmq!", for example with
+https://www.rabbitmq.com/management-cli.html[`rabbitmqadmin`]:


what does it mean here "with rabbitmqadmin"?

"Using the rabbitmqadmin tool to publish a message."

You can also send the same message by writing python, java, etc code, but having a CLI makes it easy to test out if things are working.

abrennan89 · 2021-01-27T19:53:47Z

source/README.adoc

+You will need a Kubernetes Secret to hold the RabbitMQ username and
+password. The following command is one way to create a secret with the username
+`rabbit-user` and the password taken from the `/tmp/password` file.
+----
+kubectl create secret generic rabbitmq-secret \
+  --from-literal=user=rabbit-user \
+  --from-file=password=/tmp/password
+----


This is buried for something that looks important, I don't like the structure of this "Creating and managing sources" section, because a user will come here looking for a clear procedure and instead it's a lot of confusing reference type info

Each source needs some credentials (username/password) for connecting to the RabbitMQ instance. Something along these lines will be pretty typical.

I can change the second sentence to "with kubectl:" or "For example:"; there are a lot of different ways the secret could be populated, and we've got this in here simply to show an easy way to kick the tires.

evankanderson · 2021-01-28T23:58:02Z

I tried out a different arrangement here, but I think Ashleigh is also going to try out something more dramatic in the medium-term in parallel to this.

Feel free to approve this PR or #217 as preferred.

/hold cancel

n3wscott

/lgtm
/approve

knative-prow-robot · 2021-01-30T00:14:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abrennan89, evankanderson, n3wscott

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

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

n3wscott · 2021-01-30T00:14:48Z

We will have to add a linter to knobots that knows about asciidoc as this gains traction.

Evan Anderson added 6 commits January 14, 2021 16:23

Add user documentation, move DEVELOPMENT.md

d9fa694

Add TOC

e15337e

Simplify example, fix formatting

8222bf4

Fix source blocks

731a650

Fix formatting

1f62b9f

Fix prose

ef3f7b4

knative-prow-robot added the kind/documentation label Jan 15, 2021

knative-prow-robot requested review from grantr and vaikas January 15, 2021 23:50

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 15, 2021

google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Jan 15, 2021

knative-prow-robot assigned abrennan89 and lionelvillard Jan 16, 2021

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 16, 2021

knative-prow-robot assigned n3wscott Jan 16, 2021

evankanderson commented Jan 16, 2021

View reviewed changes

vaikas reviewed Jan 18, 2021

View reviewed changes

abrennan89 reviewed Jan 18, 2021

View reviewed changes

source/README.adoc Outdated

=== Samples

(in CloudEvents JSON format):

Copy link

abrennan89 Jan 18, 2021

Choose a reason for hiding this comment

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

Avoid unnecessary parenthesis, suggest:
.CloudEvent JSON format

abrennan89 reviewed Jan 18, 2021

View reviewed changes

abrennan89 suggested changes Jan 18, 2021

View reviewed changes

evankanderson force-pushed the gold-star branch 4 times, most recently from ec0760c to 487d5b8 Compare January 26, 2021 23:03

Address comments

25cc6a2

evankanderson force-pushed the gold-star branch from 487d5b8 to 25cc6a2 Compare January 26, 2021 23:04

abrennan89 approved these changes Jan 27, 2021

View reviewed changes

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 27, 2021

abrennan89 reviewed Jan 27, 2021

View reviewed changes

evankanderson mentioned this pull request Jan 28, 2021

Alternate documentation layout #217

Closed

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 28, 2021

n3wscott reviewed Jan 30, 2021

View reviewed changes

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 30, 2021

n3wscott mentioned this pull request Jan 30, 2021

Add a linter that knows about asciidoc knative-extensions/knobots#38

Closed

knative-prow-robot merged commit daa558e into knative-extensions:master Jan 30, 2021


		== Published Events

		All messages received by the Source will be published with the following schema:

	All messages received by the Source will be published with the following schema:
	All messages received by the source are published with the following schema:


		All messages received by the Source will be published with the following schema:

		.Table Event Attributes

	\| `id` \| a unique id \| This uses the MessageId if available, and a UUID otherwise.
	\| `id` \| A unique ID \| This uses the `MessageId` if available, and a UUID otherwise.

	\| `subject` \| the message's id \| empty string if no message ID is present.
	\| `subject` \| The ID of the message \| Empty string if no message ID is present.

	\| `key` \| the message's id \| empty string if no message ID is present.
	\| `key` \| The ID of the message \| Empty string if no message ID is present.

	The event's payload will be set to the data content of the message.
	The payload of the event is set to the data content of the message.

	== Creating and Managing Sources
	== Creating and managing sources

		Note that not all parameters need to be specified; unspecified parameters will be
		defaulted false / empty.

		The standard `config-observability`, `config-logging` et al ConfigMaps may be
		used to manage the logging and metrics configuration.

	The standard `config-observability`, `config-logging` et al ConfigMaps may be
	used to manage the logging and metrics configuration.
	You can use the standard `config-observability` and `config-logging` ConfigMaps to manage the logging and metrics configuration.

	All messages received by the source are published with the following schema:
	All messages received by the RabbitMQ source are published with the following schema:

-.Event attributes
-|===
-| Attribute | Value | Notes
-| `type` | `dev.knative.rabbitmq.event` |
-| `source` | `/apis/v1/namespace/*$NS*/rabbitmqsources/*$NAME*#*$TOPIC*`
-   | `NS`, `NAME` and `TOPIC` are derived from the source configuration
-| `id` | A unique ID | This uses the `MessageId` if available, and a UUID otherwise
-| `subject` | The ID of the message | Empty string if no message ID is present
-| `datacontenttype` | `application/json` | Currently static
-| `key` | The ID of the message | Empty string if no message ID is present
-|===
-The payload of the event is set to the data content of the message.
+[source,json]
+----
+{
+  "specversion": "1.0",
+  "type": "dev.knative.rabbitmq.event",
+  "source": "/apis/v1/namespaces/<namespace>/rabbitmqsources/<name>#<topic>", <1>
+  "id": "<unique_ID>", <2>
+  "time": "<timestamp>",
+  "subject": "", <3>
+  "datacontenttype": "application/json",
+  "key": "", <4>
+  "data": "Hello rabbitmq!" <5>
+}
+----
+<1> `<namespace>`, `<name>` and `<topic>` are derived from the source configuration.
+<2> Uses the `MessageId` if available, and a UUID otherwise.
+<3> The ID of the message, or an empty string if no message ID is present.
+<4> The ID of the message, or an empty string if no message ID is present.
+<5> The payload of the event is set to the data content of the message.


		The payload of the event is set to the data content of the message.

		=== Samples

		For a message published with the payload "Hello rabbitmq!", for example with
		https://www.rabbitmq.com/management-cli.html[`rabbitmqadmin`]:

Implement best-practice Source documentation #201

Implement best-practice Source documentation #201

Conversation

evankanderson commented Jan 15, 2021

Changes

codecov bot commented Jan 15, 2021 • edited

Codecov Report

evankanderson commented Jan 16, 2021

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

vaikas 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

Choose a reason for hiding this comment

abrennan89 Jan 18, 2021 • edited

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

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

Choose a reason for hiding this comment

abrennan89 Jan 18, 2021 • edited

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

abrennan89 Jan 18, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abrennan89 left a comment

Choose a reason for hiding this comment

evankanderson commented Jan 26, 2021

abrennan89 left a comment

Choose a reason for hiding this comment

abrennan89 Jan 27, 2021 • edited

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

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

evankanderson commented Jan 28, 2021

n3wscott left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Jan 30, 2021

n3wscott commented Jan 30, 2021

codecov bot commented Jan 15, 2021 •

edited

abrennan89 Jan 18, 2021 •

edited

abrennan89 Jan 18, 2021 •

edited

abrennan89 Jan 18, 2021 •

edited

abrennan89 Jan 27, 2021 •

edited