-
Notifications
You must be signed in to change notification settings - Fork 2
feat: add namespace label filter #151
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
base: main
Are you sure you want to change the base?
Conversation
Namespace name and label will both be used to filter for namespaces. Either of them can be disabled, increasing flexibility when filtering for namespaces. Tests and functionality to come in later commits.
| for _, ns := range NsList(kube, cfg.NsLabel) { | ||
| // skip if not in configured namespace | ||
| if ns.Name != cfg.Namespace && cfg.Namespace != "" { | ||
| continue |
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.
This git diff looks weird but all I've done is wrap the red code inside the block above
|
Oh hold for now because this pr is pretty drastic. Still deciding what to do with it. |
| // given a pvc, this function will aquire the details related to the pvc such as the owner of the pvc, their email, the bounded volume name and ID, and details about its deletion | ||
|
|
||
| func EmailDetails(kube kubernetes.Interface, pvc corev1.PersistentVolumeClaim, gracePeriod int) (string, structInternal.Personalisation) { | ||
| ns := pvc.Namespace |
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.
Why change this?
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.
To match the code style from the rest of the repo. The rest of the code doesn't use temporary variables.
| API_KEY: "Random APIKEY", | ||
| */ | ||
|
|
||
| type ControllerConfig struct { |
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.
Any reason why we don't extract the common fields into their own shared config? Or why not have 1 config struct?
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.
The controller and scheduler were designed to be as loosely coupled as possible, so we didn't want to mix the configs. It would also be too unnecessarily complicated to create another struct for just a few values.
|
This PR is on hold. Maybe someone will re-implement this in the future. The issue with it currently is that it theoretically decreases runtime performance drastically by starting a goroutine for each namespace. In the future, someone may want to research better ways to implement a filter by labels, then test real-time performance. |
Proposed Changes/Description
Adds a namespace label to filter for when running controller and scheduler. The plan is to use both a name and a label to filter for namespaces. Either of then can be disabled, increasing flexibility to select namespaces. Unit tests still pending.
Types of Changes
What types of changes does your code introduce to volume-cleaner? Put an
xin the boxes that applyRelated Issue/Ticket
closes #136
Checklist