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

NETOBSERV-448: reviewing API to follow conventions #163

Merged
merged 4 commits into from
Sep 7, 2022
Merged

NETOBSERV-448: reviewing API to follow conventions #163

merged 4 commits into from
Sep 7, 2022

Conversation

mariomac
Copy link
Contributor

@mariomac mariomac commented Sep 6, 2022

  • Modified fields' documentation to follow JSON naming, according to the Openshift API Conventions' "Use JSON fields name in Godoc"
  • Breaking change: changed eBPF Agents' cacheMaxFlows default to 5000, according to the new default in the eBFP agent (1000 demonstrated to be too small for an idle 6-nodes cluster)
  • Breaking change: in the FlowCollector spec, renamed flowlogsPipeline to processor

@mariomac mariomac added documentation Improvements or additions to documentation ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. breaking-change This pull request has breaking changes. They should be described in PR description. labels Sep 6, 2022
@github-actions
Copy link

github-actions bot commented Sep 6, 2022

New image: ["quay.io/netobserv/network-observability-operator:19b42de"]. It will expire after two weeks.

jpinsonneau
jpinsonneau approved these changes Sep 7, 2022
View reviewed changes
Copy link
Contributor

@jpinsonneau jpinsonneau left a comment

Choose a reason for hiding this comment

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

LGTM. Should we ping everyone in slack about the breaking changes before merge ?

bundle/manifests/flows.netobserv.io_flowcollectors.yaml
the reporter will aggregate flows before sending
pattern: ^\d+(ns|ms|s|m)?$
type: string
cacheMaxFlows:
default: 1000
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

api/v1alpha1/flowcollector_types.go
FlowlogsPipeline FlowCollectorFLP `json:"flowlogsPipeline,omitempty"`
// processor defines the settings of the component that receives the flows from the agent,
// enriches them, and forwards them to the Loki persistence layer.
Processor FlowCollectorFLP `json:"processor,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 sounds clearer for users

@openshift-ci openshift-ci bot added the lgtm label Sep 7, 2022
@openshift-ci openshift-ci bot removed the lgtm label Sep 7, 2022
@openshift-ci
Copy link

openshift-ci bot commented Sep 7, 2022

New changes are detected. LGTM label has been removed.

@github-actions github-actions bot removed the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Sep 7, 2022
jotak
jotak reviewed Sep 7, 2022
View reviewed changes
api/v1alpha1/flowcollector_types.go Outdated
// If empty, the namespace of the operator is going to be used.
// +optional
Namespace string `json:"namespace,omitempty"`

// agent for flows' extraction.
Copy link
Member

@jotak jotak Sep 7, 2022 •
edited

Choose a reason for hiding this comment

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

nit / typo: I don't think the quote is needed here? Should be just "agent for flows extraction" ?
(If a native english speaker could confirm, cc @nalhadef / @stleerh ... )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not 100% sure but I think you are right. Fixing it and we can revert it back if Neal/Steven or anyone else says it's required.

Copy link

Choose a reason for hiding this comment

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

@mariomac @jotak I agree. The quotation marks should be removed.

@jotak
Copy link
Member

jotak commented Sep 7, 2022

just a minor comment (cf above), else lgtm

@openshift-ci
Copy link

openshift-ci bot commented Sep 7, 2022

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by:

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

@mariomac mariomac added the lgtm label Sep 7, 2022
@mariomac mariomac merged commit aaf5a57 into netobserv:main Sep 7, 2022
@mariomac mariomac deleted the review-api branch September 7, 2022 11:03
KalmanMeth pushed a commit to KalmanMeth/network-observability-operator that referenced this pull request Feb 13, 2023
@eranra
Merge pull request netobserv#163 from eranra/improve_dockerfile
c274cb4 
add mod download also for lint
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jotak jotak jotak left review comments

@nalhadef nalhadef nalhadef left review comments

@jpinsonneau jpinsonneau jpinsonneau approved these changes

Assignees

@jpinsonneau jpinsonneau

Labels
approved breaking-change This pull request has breaking changes. They should be described in PR description. documentation Improvements or additions to documentation lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@mariomac @jotak @nalhadef @jpinsonneau @openshift-merge-robot