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
Add label selector to exclude deployments and cronjobs #220
Conversation
LGTM |
Pull Request Test Coverage Report for Build 3614548454
💛 - Coveralls |
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.
Hi @dirk39, thank you for the PR. Great work!
I didn't know the Open Source Saturday meetup, very interesting!
Here a couple of comments.
The implementation of the CronJob differs because it uses the FieldSelector query, do you mean this? This should be more performant, as it permits to get only the resources needed. It is not possible to add this filter using the FieldSelector
?
Otherwise, yes I suppose that it is possible to change the implementation to add this filter.
Hi @davidebianchi , the general idea was to uniform the behavior of the deployment and cronjob filter. I think that the deployment solution is easier to maintain but if you prefer I can have a look to the fieldSelector to see if they support the labelMatch. |
CronJobs have been made after Deployments, so when I developed the Deployments I didn't know the fieldSelector. But with the labels, there might be a large number of Deployments and CronJobs to exclude so it could be better to use it. So yes, if you want to see for the support it would be better, but also this implementation is ok for me. |
Co-authored-by: ludusrusso <ludus.russo@gmail.com>
items Co-authored-by: ludusrusso <ludus.russo@gmail.com>
Hi @dirk39, is it the introduction of labelSelector finished? I see that the last commit contains is in wip. |
Hi @davidebianchi , I've introduced the labelselector for the cronjob but I have to add tests to validate the solution. |
Signed-off-by: Andrea Quintino <andrea.quintino@alirahealth.com>
Signed-off-by: Andrea Quintino <andrea.quintino@alirahealth.com>
Hi @davidebianchi, I'm sorry for my delay. I've added unit and e2e tests for the CJ. Let me know if it looks good to you |
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.
LGTM!
Thanks for the contribution, great work!
Hello, with this PR we want to fix issue #172. We noticed that the behaviour of the Cronjob selector differs from the deployment. Do you think we can replicate the deployment behaviour in the cronjob? So we can add the matchLabels functionality.