-
Notifications
You must be signed in to change notification settings - Fork 24
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
Doc updates: #370
Doc updates: #370
Conversation
jotak
commented
Jun 9, 2023
- Generalize using backticks for cross-references of spec fields, enum values, etc.
- Format enum options as list when their description isn't too short
- A few rephrasing (e.g. in conversation tracking)
- should -> must
api/v1beta1/flowcollector_types.go
Outdated
// logTypes defines the desired record types to generate. Possible values are:<br> | ||
// - `FLOWS` (default) to export regular network flows<br> | ||
// - `CONVERSATIONS` to generate events for started conversations, ended conversations as well as periodic "tick" updates<br> | ||
// - `ENDED_CONVERSATIONS` to generate only ended conversations events<br> | ||
// - `ALL` to generate both network flows and all conversations events<br> | ||
// +kubebuilder:validation:Optional | ||
// +kubebuilder:validation:Enum:="FLOWS";"CONVERSATIONS";"ENDED_CONVERSATIONS";"ALL" | ||
// +kubebuilder:default:=FLOWS | ||
LogTypes *string `json:"logTypes,omitempty"` | ||
|
||
//+kubebuilder:default:="30s" | ||
// +optional | ||
// conversation heartbeat interval is the duration of time to wait between heartbeat reports of a conversation | ||
// conversation heartbeat interval is the duration of time to wait between "tick" events of a conversation | ||
ConversationHeartbeatInterval *metav1.Duration `json:"conversationHeartbeatInterval,omitempty"` | ||
|
||
//+kubebuilder:default:="10s" | ||
// +optional | ||
// conversation end timeout is the duration of time to wait from the last flow log to end a conversation | ||
// conversation end timeout is the time to wait after a network flow is received, to consider the conversation ended. | ||
// This delay is ignored when a FIN packet is collected for TCP flows (see `conversationTerminatingTimeout` instead). | ||
ConversationEndTimeout *metav1.Duration `json:"conversationEndTimeout,omitempty"` | ||
|
||
//+kubebuilder:default:="5s" | ||
// +optional | ||
// conversation terminating timeout is the duration of time to wait from detected FIN flag to end a connection | ||
// conversation terminating timeout is the time to wait from detected FIN flag to end a conversation. Only relevant for TCP flows. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jpinsonneau can you review in particular this part? Is this all correct and makes sense?
I refer to "ticks" here since, if I'm right, this is how it is mentioned in the UI
Also added some precisions on the timeouts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes the frontend translate the events as:
switch (f.labels._RecordType) {
case 'newConnection':
return t('Conversation start');
case 'heartbeat':
return t('Conversation tick');
case 'endConnection':
return t('Conversation end');
case 'flowLog':
default:
return t('Flow');
}
@skrthomas I'll try also to fix something else in a next commit: our asciidoc generator doesn't handle very well the arrays kind of fields, e.g. "exporters" appears twice, first as "exporters | array" and then again as "exporters[] | object" - and only the description of the former is meant to be rendered. I'll see if I can fix that as a post-processing. |
Codecov Report
@@ Coverage Diff @@
## main #370 +/- ##
==========================================
- Coverage 53.58% 53.54% -0.05%
==========================================
Files 45 45
Lines 5488 5381 -107
==========================================
- Hits 2941 2881 -60
+ Misses 2337 2294 -43
+ Partials 210 206 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant to publish these comments on Friday @jotak !! I finished reviewing and have a few things I noticed throughout. I've incorporated these new updates into the product doc.
thanks @skrthomas , all comments addressed |
@jotak I just got my peer review back and it seems like we have some more edits to make. Nothing big mostly just replacing may/might, some `` needed around parameters, commas, things like that. |
// "IPFIX" works with OVN-Kubernetes CNI (other CNIs could work if they support exporting IPFIX, | ||
// type selects the flows tracing agent. Possible values are:<br> | ||
// - `EBPF` (default) to use NetObserv eBPF agent.<br> | ||
// - `IPFIX` - <i>deprecated (*)</i> - to use the legacy IPFIX collector.<br> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it deprecated or unsupported for 1.3+ ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this one was ambiguous as it was deprecated before even our 1st GA ... but considering the history of the project upstream I would say it's deprecated, since it was once the only available option
// AuthToken describe the way to get a token to authenticate to Loki.<br> | ||
// - `DISABLED` will not send any token with the request.<br> | ||
// - `FORWARD` will forward the user token for authorization.<br> | ||
// - `HOST` - <i>deprecated (*)</i> - will use the local pod service account to authenticate to Loki.<br> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here: deprecated or unsupported for 1.3+ ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I think it's clearly deprecated: it used to be a supported feature, and is not anymore
@skrthomas I applied the feedback from downstream, with the exception of the backticks on the field name being described (this one: openshift/openshift-docs#61006 (comment) ) : because it's a pattern we use almost everywhere in the file and not just at the place where it was suggested, if we want to do this change we should do it everywhere for consistency. So at the moment I haven't done the change ... but yes we could do it everywhere. |
@jotak OK, I recopied your file. If it isn't too much trouble to regenerate with the backticks for the field name descriptions, lets go ahead and do that. |
@skrthomas I'm not entirely sure yet how to proceed. We have 2 kinds of patterns:
In this case it seems legit to use the backticks:
In this case it seems more "accidental" that the word is the same as the API field. This could be a problem for instance with this one:
"Kafka" should take a capital K, unless it's referencing the field in which case perhaps we should reformulate using the 1. pattern:
But it creates more repetitions so it's not pretty. |
@skrthomas my last two commits include generalization of backticks, and reintroducing some lost rephrasing that were done only downstream (as shown in the current diff https://github.com/openshift/openshift-docs/pull/61006/files ) |
@jotak I think that the implicit references, like scenario 2, could be reformulated to be more like scenario 1. For example, it could be For Kafka, Ah, ok thanks for clarifying the capitalization situational differences. And I agree, it can be reformulated also more like the 1 pattern. |
/lgtm |
@@ -37,7 +37,7 @@ Type:: | |||
| `object` | |||
| FlowCollectorSpec defines the desired state of FlowCollector. + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| FlowCollectorSpec defines the desired state of FlowCollector. + | |
| spec defines the desired state of FlowCollector. + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be just spec
? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed the mention of FlowCollectorSpec, but there must be other places with the same issue (that's not new btw, it was already like this previously)
- Generalize using backticks for cross-references of spec fields, enum values, etc. - Format enum options as list when their description isn't too short - A few rephrasing (e.g. in conversation tracking) - should -> must Remove some duplicate entries Apply suggestions from code review Co-authored-by: Sara Thomas <sarthoma@redhat.com> Regenerate doc Address downstream comments Update api/v1beta1/flowcollector_types.go Co-authored-by: Julien Pinsonneau <91894519+jpinsonneau@users.noreply.github.com> Generalize use of backticks Reintroduce some changes that were only downstream
New changes are detected. LGTM label has been removed. |
/approve |
[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 |