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

feat: extending filtering options #52

Merged
merged 1 commit into from
Apr 11, 2022
Merged

feat: extending filtering options #52

merged 1 commit into from
Apr 11, 2022

Conversation

slayer321
Copy link
Contributor

@slayer321 slayer321 commented Mar 20, 2022 •
edited
Loading

Signed-off-by: slayer321 [email protected]

Add 7 filtering options

--namespace
--operation
--logType
--container
--pod
--resource
--source

related issue #40

cc @nyrahul @daemon1024

nyrahul
nyrahul requested changes Mar 21, 2022
View reviewed changes
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.

Nice work @slayer321. Few comments:

  • Avoid code duplication
  • Compile the regex just once
  • Add unit tests

log/logClient.go Outdated
@@ -204,6 +205,39 @@ func (fd *Feeder) WatchAlerts(logPath string, jsonFormat bool) error {
break
}

if namespace != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Few comments:

  • Compile has to be done just once... not on every event.
  • avoid code duplication
  • add some unit tests

daemon1024
daemon1024 requested changes Mar 21, 2022
View reviewed changes
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.

Thanks for the PR, Looks like a great start 😄

Left some comments in addition to what @nyrahul suggested.

log/log.go Outdated Show resolved Hide resolved
log/logClient.go Outdated Show resolved Hide resolved
slayer321
slayer321 commented Mar 25, 2022
View reviewed changes
cmd/log.go
logCmd.Flags().StringVar(&logOptions.ContainerName, "containerName", "", "name of the container ")
logCmd.Flags().StringVar(&logOptions.PodName, "podName", "", "name of the pod ")
logCmd.Flags().StringVar(&logOptions.Resource, "resource", "", "command used by the user")
logCmd.Flags().StringVar(&logOptions.Source, "source", "", "binary used by the system ")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

resource and source usage might not be correct ...
correct me with that

@daemon1024 daemon1024 self-requested a review March 25, 2022 16:44
daemon1024
daemon1024 requested changes Mar 25, 2022
View reviewed changes
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.

If the values/names of particular fields can only be in lower case then it should be fine to make the regex match case insensitive for them since it wouldn't matter what case we provide in filter, the resultant value is only in one case. Does this make sense?

Left some more comments in the review. Thank You.

cmd/log.go Outdated Show resolved Hide resolved
log/logClient.go Outdated Show resolved Hide resolved
log/logClient.go Outdated Show resolved Hide resolved
@daemon1024 daemon1024 requested a review from nyrahul March 30, 2022 12:07
daemon1024
daemon1024 requested changes Mar 30, 2022
View reviewed changes
log/logClient.go Outdated Show resolved Hide resolved
@slayer321
Copy link
Contributor Author

Hey @daemon1024, I have made the required changes. Do check and let me know your feedback.
For case insensitive of both the fields(--operation,--logType) I have directly given the string and removed the boolean part .. as we are compiling the regex before passing it to both the functions

daemon1024
daemon1024 requested changes Apr 3, 2022
View reviewed changes
log/log.go Outdated Show resolved Hide resolved
log/log.go Outdated Show resolved Hide resolved
daemon1024
daemon1024 reviewed Apr 3, 2022
View reviewed changes
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.

Nice Work 👌🏽 Can you handle the linter errors (w.r.t your changes) and squash the commits. Thank You

@slayer321
Copy link
Contributor Author

I'm not able to see any linter error ... It is showing all checks have passed

@daemon1024
Copy link
Member

daemon1024 commented Apr 3, 2022 •
edited
Loading

@slayer321 linter warnings*
You can check them in the files tab

Screenshot_2022-04-03-20-21-45-89_40deb401b9ffe8e1df2f1cc5ba480b12.jpg

nyrahul
nyrahul requested changes Apr 6, 2022
View reviewed changes
log/logClient.go Outdated Show resolved Hide resolved
log/logClient.go Outdated Show resolved Hide resolved
@daemon1024 daemon1024 requested a review from nyrahul April 8, 2022 15:50
nyrahul
nyrahul approved these changes Apr 11, 2022
View reviewed changes
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.

Thanks for the PR. LGTM.

@daemon1024 daemon1024 merged commit bd242cc into kubearmor:main Apr 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daemon1024 daemon1024 daemon1024 approved these changes

@nyrahul nyrahul nyrahul approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@slayer321 @daemon1024 @nyrahul