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 & remove default filter(s) to analyze. #161

Merged

Conversation

matthisholleville
Copy link
Contributor

Closes #158

πŸ“‘ Description

This MR allows the addition of custom default filters. As discussed here #158, it would be interesting for the user to have the possibility to manage the analyzers they are interested in and for example not have to execute PDB analyzers when they do not use them.

βœ… Checks

  • My pull request adheres to the code style of this project
  • My code requires changes to the documentation
  • I have updated the documentation as required
  • All the tests have passed

β„Ή Additional Information

I have done a lot of testing of adding/removing filters in the configuration file and have not found any side effects. If you would like me to perform additional tests, please let me know which ones.

This version does not include a breaking change.

@matthisholleville matthisholleville requested a review from a team as a code owner March 30, 2023 20:10
README.md Outdated Show resolved Hide resolved
Signed-off-by: Matthis Holleville <matthish29@gmail.com>
cmd/filters/filtersAdd.go Outdated Show resolved Hide resolved
cmd/filters/filtersAdd.go Outdated Show resolved Hide resolved
cmd/filters/filtersRemove.go Outdated Show resolved Hide resolved
cmd/filters/filtersAdd.go Outdated Show resolved Hide resolved
cmd/filters/filtersAdd.go Outdated Show resolved Hide resolved
@AlexsJones
Copy link
Member

Available filters :
> Service
> Ingress
> Pod
> ReplicaSet
> PersistentVolumeClaim
alex@alpha:~/Code/k8sgpt$ go run main.go filters remove ReplicaSet
Filter(s) ReplicaSet does not exist in configuration file. Please use k8sgpt filters add.
exit status 1

@AlexsJones
Copy link
Member

It looks like this would be a breaking change, we would have to ask people to regenerate their k8sgpt.yaml?

@matthisholleville
Copy link
Contributor Author

No breaking change. I handle the fact that the default_filters key does not exist in the file.

Signed-off-by: Matthis Holleville <matthish29@gmail.com>
Signed-off-by: Matthis Holleville <matthish29@gmail.com>
@matthisholleville
Copy link
Contributor Author

#161 (comment)

This error is correct. You are trying to remove a filter that is not defined in your configuration file.
Isn't the error message precise enough?

➜  k8sgpt git:(feature/add-and-remove-filters) ./k8sgpt filters list
Available filters : 
> ReplicaSet
> PersistentVolumeClaim
> Service
> Ingress
> Pod
➜  k8sgpt git:(feature/add-and-remove-filters) ./k8sgpt filters add ReplicaSet
Filter ReplicaSet added
➜  k8sgpt git:(feature/add-and-remove-filters) ./k8sgpt filters remove ReplicaSet
Filter(s) ReplicaSet removed

Signed-off-by: Matthis Holleville <matthish29@gmail.com>
@AlexsJones
Copy link
Member

AlexsJones commented Mar 31, 2023

#161 (comment)

This error is correct. You are trying to remove a filter that is not defined in your configuration file. Isn't the error message precise enough?

➜  k8sgpt git:(feature/add-and-remove-filters) ./k8sgpt filters list
Available filters : 
> ReplicaSet
> PersistentVolumeClaim
> Service
> Ingress
> Pod
➜  k8sgpt git:(feature/add-and-remove-filters) ./k8sgpt filters add ReplicaSet
Filter ReplicaSet added
➜  k8sgpt git:(feature/add-and-remove-filters) ./k8sgpt filters remove ReplicaSet
Filter(s) ReplicaSet removed

Okay, so maybe this is a DX issue.
Here is the assumption I have:

As a user when I run K8sGPT I'll have some sensible defaults enabled e.g.

Active:
> > ReplicaSet
> > PersistentVolumeClaim
> > Service
> > Ingress
> > Pod

Let's say I actually don't find Ingress information useful and just spammy in my cluster.
I then say okay k8sgpt filter remove Ingress
That then gives me:

Active:
> > ReplicaSet
> > PersistentVolumeClaim
> > Service
> > Pod
Unused:
> > Ingress

Maybe I didn't articulate this, but it seems we have the "built in" concept vs the "user defined default list"

WDYT?

Signed-off-by: Matthis Holleville <matthish29@gmail.com>
@matthisholleville
Copy link
Contributor Author

@AlexsJones That's very clear and makes sense. I'll make you an implementation proposal.

Signed-off-by: Matthis Holleville <matthish29@gmail.com>
@matthisholleville
Copy link
Contributor Author

@AlexsJones New version available. I have taken your requests into account. Below you will find my tests:

➜  k8sgpt git:(feature/add-and-remove-filters) βœ— cat ~/.k8sgpt.yaml
backend_type: openai
kubernetesclient: {}
openai_key: xxxxxx                                                                                                     
➜  k8sgpt git:(feature/add-and-remove-filters) βœ— ./k8sgpt filters list
Active: 
> Service
> Ingress
> Pod
> ReplicaSet
> PersistentVolumeClaim
➜  k8sgpt git:(feature/add-and-remove-filters) βœ— ./k8sgpt filters add Pod
Duplicate filters found: Pod
➜  k8sgpt git:(feature/add-and-remove-filters) βœ— ./k8sgpt filters remove Pod
Filter(s) Pod removed
➜  k8sgpt git:(feature/add-and-remove-filters) βœ— cat ~/.k8sgpt.yaml         
active_filters:
    - ReplicaSet
    - PersistentVolumeClaim
    - Service
    - Ingress
backend_type: openai
kubernetesclient: {}
openai_key: xxxxxx

I also took the opportunity to rename the concept of default_filters to active_filters, which seems more meaningful to me.

@AlexsJones
Copy link
Member

This is an awesome contribution thank you @matthisholleville

Copy link
Member

@AlexsJones AlexsJones left a comment

Choose a reason for hiding this comment

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

πŸ‘πŸ½

@AlexsJones AlexsJones merged commit c2b8732 into k8sgpt-ai:main Mar 31, 2023
5 checks passed
fyuan1316 pushed a commit to fyuan1316/k8sgpt that referenced this pull request Jun 26, 2023
Signed-off-by: Alex Jones <alexsimonjones@gmail.com>
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.

feature: default filters vs opt-in
3 participants