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.

Sign up for GitHub

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

Jump to bottom

CRD doc updates #364

Merged
merged 2 commits into from
Jun 7, 2023
Merged

CRD doc updates #364

merged 2 commits into from
Jun 7, 2023

Conversation

jotak
Copy link
Member

@jotak jotak commented Jun 6, 2023

  • clarify "deprecated" and "unsupported" features
  • fix/remove a forgotten mention that kafka export is experimental
  • reformulate the API description (it also appears on the CSV/operator description)
  • a couple of minor fixes
  • regenerate docs, update asciidoc

@jotak
CRD doc updates
b305102 
- clarify "deprecated" and "unsupported" features
- fix/remove a forgotten mention that kafka export is experimental
- reformulate the API description (it also appears on the CSV/operator
  description)
- a couple of minor fixes
- regenerate docs, update asciidoc
@jotak
Copy link
Member Author

jotak commented Jun 6, 2023

FYI I've updated the text of the API description, that is also visible when installing / configuring the operator:

image
(this pic is of the old text)

I find the old text not very good: it's repetitive and doesn't really says anything. I'm proposing this instead:

FlowCollector is the schema for the network flows collection API, which pilots and configures the underlying deployments.

Also I'm including an explanation of what means "unsupported" etc.

@jotak
Copy link
Member Author

jotak commented Jun 6, 2023

@skrthomas it includes the re-generation of the asciidoc api reference

@codecov
Copy link

codecov bot commented Jun 6, 2023 •
edited

Codecov Report

Merging #364 (b305102) into main (f664d0f) will not change coverage.
The diff coverage is n/a.

❗ Current head b305102 differs from pull request most recent head 0d7db7a. Consider uploading reports for the commit 0d7db7a to get more accurate results

@@           Coverage Diff           @@
##             main     #364   +/-   ##
=======================================
  Coverage   53.53%   53.53%           
=======================================
  Files          45       45           
  Lines        5329     5329           
=======================================
  Hits         2853     2853           
  Misses       2272     2272           
  Partials      204      204
Flag Coverage Δ
unittests 53.53% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
api/v1beta1/flowcollector_types.go 100.00% <ø> (ø)

memodi
memodi approved these changes Jun 6, 2023
View reviewed changes
Copy link
Contributor

@memodi memodi left a comment

Choose a reason for hiding this comment

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

/label qe-approved

@openshift-ci openshift-ci bot added qe-approved QE has approved this pull request lgtm labels Jun 6, 2023
@jotak
Copy link
Member Author

jotak commented Jun 7, 2023

/approve
I'll backport to 1.3

@openshift-ci
Copy link

openshift-ci bot commented Jun 7, 2023

[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-ci openshift-ci bot added the approved label Jun 7, 2023
@openshift-merge-robot openshift-merge-robot merged commit ff69bf3 into netobserv:main Jun 7, 2023
7 checks passed
skrthomas
skrthomas reviewed Jun 7, 2023
View reviewed changes
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.

LGTM!

skrthomas
skrthomas reviewed Jun 8, 2023
View reviewed changes
docs/flowcollector-flows-netobserv-io-v1beta1.adoc
@@ -845,7 +900,7 @@ Type::

| `authToken`
| `string`
| AuthToken describe the way to get a token to authenticate to Loki. DISABLED will not send any token with the request. HOST will use the local pod service account to authenticate to Loki. FORWARD will forward user token, in this mode, pod that are not receiving user request like the processor will use the local pod service account. Similar to HOST mode. When using the Loki Operator, set it to `HOST` or `FORWARD`.
| AuthToken describe the way to get a token to authenticate to Loki. DISABLED will not send any token with the request. HOST - _deprecated (*)_ - will use the local pod service account to authenticate to Loki. FORWARD will forward the user token for authorization. When using the Loki Operator, this should be set to `FORWARD`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Really minor thing, but IBM Style Guide says to use "must" instead of "should", so is it OK to change this one small thing in the docs version of this 😬 ?

Copy link
Member Author

Choose a reason for hiding this comment

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

@skrthomas I'll send a new PR with this + a couple of other changes

@jotak jotak deleted the doc-update branch June 9, 2023 08:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skrthomas skrthomas skrthomas left review comments

@OlivierCazade OlivierCazade OlivierCazade approved these changes

@memodi memodi memodi approved these changes

@jpinsonneau jpinsonneau Awaiting requested review from jpinsonneau

Assignees

@OlivierCazade OlivierCazade

@memodi memodi

Labels
approved lgtm qe-approved QE has approved this pull request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@jotak @OlivierCazade @skrthomas @memodi @openshift-merge-robot