-
Notifications
You must be signed in to change notification settings - Fork 612
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
chore: cleanup duplicated filter logic #402
Conversation
865365b
to
86684af
Compare
@matthisholleville @AlexsJones can you pls review it ? |
Hi, Maintainers, when you have time, can you please kindly help to review it ? |
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.
Thank you for your contribution. Great work, left a comment.
Shouldn't add some test about filter logic ?
T_T... @matthisholleville |
3fa134b
to
f6f77b0
Compare
Active filters correspond to the filters added by the command k8sgpt filter add ... Why would we need Trivy? |
@matthisholleville
For case 2, there's only Trivy plugin implemented. that's where my problem came from. in
So I guess I should do below in UT:
should I add a new public function to modify that private variable ? |
I don't quite understand point two. Active filters are the result of adding filters via the command For me there are 3 things to test:
|
Signed-off-by: Peter Pan <Peter.Pan@daocloud.io>
Signed-off-by: Peter Pan <Peter.Pan@daocloud.io>
Thanks @matthisholleville . I thought the the PR has been updated. please kindly help to review again :-) |
Fixed by : #431 |
Hi, @matthisholleville , I think you meant to reply to PR #413 (which I will close) instead of this one ? this PR has been updated with unit-test added per your ask. when you have time, can you review this ? |
We have just discussed your PR, and we believe that your modification adds more complexity than it resolves. Thank you very much for your time and contribution, but at the moment, we will not be proceeding with your PR. |
@matthisholleville , Thanks for your time and response
|
Mat explain that the filter part code is unstable now , it's better to keep it as it is. I will create new PR for UT only. |
Closes #
Clean up duplicated code of original https://github.com/k8sgpt-ai/k8sgpt/blob/main/pkg/analysis/analysis.go#L136 and https://github.com/k8sgpt-ai/k8sgpt/blob/main/pkg/analysis/analysis.go#L161
📑 Description
✅ Checks
1)I've run
make test
make fmt
make lint
make build
2)then run the generated binary with below command:
k8sgpt analyze --filter=VulnerabilityReport
k8sgpt analyze
the result outputs are the same with released binary from github.
ℹ Additional Information