-
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
Regenerate docs #657
Regenerate docs #657
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@@ -9,7 +9,7 @@ The "Filter ID" column shows which related name to use when defining Quick Filte | |||
|
|||
The "Loki label" column is useful when querying Loki directly: label fields need to be selected using link:https://grafana.com/docs/loki/latest/logql/log_queries/#log-stream-selector[stream selectors]. | |||
|
|||
The "Cardinality" column gives information about the implied metric cardinality if this field is used as a Prometheus label with the `FlowMetrics` API. Refer to the `FlowMetrics` documentation for more information on using this API. | |||
The "Cardinality" column gives information about the implied metric cardinality if this field was to be used as a Prometheus label with the `FlowMetrics` API. Refer to the `FlowMetrics` documentation for more information on using this API. |
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.
@skrthomas this change is not intentional from me, but happened when I regenerated the doc. I can't remember if the desired formulation is "is used" or "was to be used".
Side remark on the flows format reference: in the docs the page title is "Network flows format reference" but the menu item is "JSON flows format reference". Could we use the former only? It doesn't make much sense today to mention "JSON", because that's not only used in JSON but also in other formats such as prometheus labels and soon open telemetry
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 is really minor, but "Is used" is preferable just because "was to be" is passive and past tense.
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.
ACK, will make sure these match to the "Network flows format reference"
/lgtm |
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.
/label qe-approved
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.
Hey @jotak there were a few things I noticed in my review that need to be addressed. Thank you!
- `IPFIX` [deprecated (*)] - to use the legacy IPFIX collector. + | ||
`eBPF` is recommended as it offers better performances and should work regardless of the CNI installed on the cluster. `IPFIX` works with OVN-Kubernetes CNI (other CNIs could work if they support exporting IPFIX, but they would require manual configuration). | ||
| `type` [deprecated (*)] selects the flows tracing agent. Previously, this field allowed to select between `eBPF` or `IPFIX`. | ||
Only `eBPF` is allowed now, so this field is deprecated and will be removed in a future version of the API. |
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.
Only `eBPF` is allowed now, so this field is deprecated and will be removed in a future version of the API. | |
Only `eBPF` is allowed now, so this field is deprecated and is planned for removal in a future version of the API. |
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.
The RH Style Guidelines ask us to talk about future releases as "planned" instead of as "will happen"
|
||
| `imagePullPolicy` | ||
| `string` | ||
| `imagePullPolicy` is the Kubernetes pull policy for the image defined above | ||
|
||
| `interfaces` | ||
| `array (string)` | ||
| `interfaces` contains the interface names from where flows are collected. If empty, the agent fetches all the interfaces in the system, excepting the ones listed in ExcludeInterfaces. An entry enclosed by slashes, such as `/br-/`, is matched as a regular expression. Otherwise it is matched as a case-sensitive string. | ||
| `interfaces` contains the interface names from where flows are collected. If empty, the agent | ||
fetches all the interfaces in the system, excepting the ones listed in ExcludeInterfaces. |
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.
fetches all the interfaces in the system, excepting the ones listed in ExcludeInterfaces. | |
fetches all the interfaces in the system, excepting the ones listed in `excludeInterfaces`. |
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.
Since this references another parameter, can we put it in backticks? Also I notice the capitalization here is maybe needing a lowercase e
.
| `integer` | ||
| `cacheMaxFlows` is the max number of flows in an aggregate; when reached, the reporter sends the flows. | ||
| The prometheus HTTP port |
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.
| The prometheus HTTP port | |
| The Prometheus HTTP port |
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 think Prometheus always be capitalized.
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.
Actually changing this to "The metrics server HTTP port" , that's more correct
|
||
| `lokiStack` | ||
| `object` | ||
| Loki configuration for `LokiStack` mode. This is useful for an easy loki-operator configuration. It is ignored for other modes. | ||
| Loki configuration for `LokiStack` mode. This is useful for an easy loki-operator configuration. |
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.
| Loki configuration for `LokiStack` mode. This is useful for an easy loki-operator configuration. | |
| Loki configuration for `LokiStack` mode. This is useful for an easy Loki Operator configuration. |
@@ -1225,7 +1613,8 @@ Type:: | |||
Description:: | |||
+ | |||
-- | |||
Loki configuration for `LokiStack` mode. This is useful for an easy loki-operator configuration. It is ignored for other modes. | |||
Loki configuration for `LokiStack` mode. This is useful for an easy loki-operator configuration. |
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.
Loki configuration for `LokiStack` mode. This is useful for an easy loki-operator configuration. | |
Loki configuration for `LokiStack` mode. This is useful for an easy Loki Operator configuration. |
|
||
| `requests` | ||
| `integer-or-string` | ||
| Requests describes the minimum amount of compute resources required. If Requests is omitted for a container, it defaults to Limits if that is explicitly specified, otherwise to an implementation-defined value. Requests cannot exceed Limits. More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/ | ||
| Requests describes the minimum amount of compute resources required. | ||
If Requests is omitted for a container, it defaults to Limits if that is explicitly specified, |
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.
If Requests is omitted for a container, it defaults to Limits if that is explicitly specified, | |
If Requests is omitted for a container, it defaults to `limits` if that is explicitly specified, |
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.
that one is not on us it's from the upstream k8s doc
| Requests describes the minimum amount of compute resources required. If Requests is omitted for a container, it defaults to Limits if that is explicitly specified, otherwise to an implementation-defined value. Requests cannot exceed Limits. More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/ | ||
| Requests describes the minimum amount of compute resources required. | ||
If Requests is omitted for a container, it defaults to Limits if that is explicitly specified, | ||
otherwise to an implementation-defined value. Requests cannot exceed Limits. |
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.
otherwise to an implementation-defined value. Requests cannot exceed Limits. | |
otherwise to an implementation-defined value. The value of `requests` cannot exceed the value of `limits`. |
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.
Can we add backticks and a tiny amount of clarifying text?
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, that's upstream k8s
New changes are detected. LGTM label has been removed. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #657 +/- ##
=======================================
Coverage 66.53% 66.53%
=======================================
Files 70 70
Lines 8095 8095
=======================================
Hits 5386 5386
Misses 2315 2315
Partials 394 394
Flags with carried forward coverage won't be shown. Click here to find out more.
|
@skrthomas last commit should address your comments; except for what is in the upstream k8s doc |
/lgtm thank you, @jotak |
* Regenerate docs * Add flowmetrics doc * address feedback * Update flowmetrics desc
No description provided.