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.

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

feat: add limit flag #59

Merged
merged 2 commits into from
May 4, 2022
Merged

feat: add limit flag #59

merged 2 commits into from
May 4, 2022

Conversation

slayer321
Copy link
Contributor

Signed-off-by: slayer321 sachin.maurya7666@gmail.com

add limit flag

--limit

related issue #40

cc @nyrahul @daemon1024

Signed-off-by: slayer321 <sachin.maurya7666@gmail.com>
Copy link
Member

@daemon1024 daemon1024 left a comment

Choose a reason for hiding this comment

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

I am a bit skeptical about this approach. Would Atomic Counters be more feasible here and can avoid this much refactoring?

@daemon1024 daemon1024 requested a review from nyrahul April 20, 2022 10:38
Copy link
Contributor

@nyrahul nyrahul left a comment

Choose a reason for hiding this comment

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

LGTM. One comment provided inline. Please check if it can be handled.


} else {
for fd.Running {
res, err := fd.alertStream.Recv()
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the possible to remove this code duplication ... I mean Recv() ... watchAlertsHelper()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think currently only code duplication section is this one

res, err := fd.logStream.Recv()
if err != nil {
	fmt.Printf("Failed to receive an alert (%s)\n", err.Error())
	break
}
_ = WatchLogsHelper(res, o)

I tried to add it in another function ... but it didn't help

@nyrahul
Copy link
Contributor

nyrahul commented Apr 22, 2022

Can you please fix the conflict by rebasing and please check the review comment ? Thanks

@nyrahul nyrahul merged commit b61b333 into kubearmor:main May 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants