-
Notifications
You must be signed in to change notification settings - Fork 385
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(controllers): add support for instanceMatchSelector #1497
Conversation
Hi @NissesSenap ! Can you tell me your process for PR validation and testing ? I'm really interested to become active contributor on this and other Grafana projects. |
Hi @Ronan-WeScale , I can only answer for grafana-operator. We are always looking for contributors to the project, there are a number of issues that are open with help wanted flag, but to honest we want help with most of things ;) You are also welcome to join our meetings that have once a week, for more info see: https://github.com/grafana/grafana-operator?tab=readme-ov-file#get-in-touch I have kind of much to do at work right now, but I hope me or some other maintainer will have time to review this PR soon. |
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.
@Ronan-WeScale I'm happy to see this functionality implemented, though I think it's a bit early for this PR to be merged. There are two points of improvement that I'd like to highlight.
First of all, I believe the code can and should be simplified. - Ideally, I want to be able to understand the behaviour by reading the code once. In its current form, even after reading it multiple times, I'm not entirely sure I fully understand it:
- There are many if-else blocks:
- e.g. when filtering by operator, I would probably go either with switch block or just with multiple if blocks - you have
break
logic anyway;
- e.g. when filtering by operator, I would probably go either with switch block or just with multiple if blocks - you have
- default values should be explicit and static:
- e.g. there's
valueMatched := matchExpression.Operator == "NotIn"
and a for-loop afterwards - it's hard to guess the exact effect of that comparison; - I think it would make sense to set
selected := false
by default and also explicitly change it to eitherfalse
ortrue
in if blocks;
- e.g. there's
- I'm not sure if it's worth having logic from
labelMatchedExpression
in a separate function, it'd be easier to say once PR is rewritten; - Since we're using golang 1.21, you can rely on
slices.Contains
when searching for elements.
Second, I see that you added some CRs to prove that the code works as expected, but, in general case, everything that can be covered by unit tests should be done with unit tests, especially with such complex functions :) - It takes a fraction of a second to run those, and refactoring becomes super-easy (any manual step has potential to turn into a mistake). Plus, tests not only prove that something works well, but also explain the behaviour.
@Ronan-WeScale Just to give you an example on how code can change (not saying that the piece of code is entirely correct, haven't tested it - it's just to give you a direction): With if: selected := false
for _, matchExpression := range labelSelector.MatchExpressions {
if label, ok := instance.Labels[matchExpression.Key]; ok {
if matchExpression.Operator == "DoesNotExist" {
selected = false
}
if matchExpression.Operator == "Exists" {
selected = true
}
if matchExpression.Operator == "In" {
selected = slices.Contains(matchExpression.Values, label)
}
if matchExpression.Operator == "NotIn" {
selected = !slices.Contains(matchExpression.Values, label)
}
// All matchExpressions must evaluate to true in order to satisfy the conditions
if !selected {
break
}
}
} With switch: selected := false
for _, matchExpression := range labelSelector.MatchExpressions {
if label, ok := instance.Labels[matchExpression.Key]; ok {
switch matchExpression.Operator {
case "DoesNotExist":
selected = false
case "Exists":
selected = true
case "In":
selected = slices.Contains(matchExpression.Values, label)
case "NotIn":
selected = !slices.Contains(matchExpression.Values, label)
}
// All matchExpressions must evaluate to true in order to satisfy the conditions
if !selected {
break
}
}
} So, my suggestion would be to extract the match selector logic to a separate function, so you can write some tests without the need for mocking k8s API-server and then prepare something similar to what I showed above (it doesn't have to be exactly the same code, I'm simply looking for something with low complexity, so it'd be easy to further maintain). I hope that helps :) Upd: One more thing that is missing (at least, per my understanding) is that all expressions must evaluate to true, so we should not really run |
Hi @weisdd ! Thanks for your answer, I worked on e2e test last week. |
Hi @weisdd @NissesSenap, I changed the code, and run e2e test, sorry but I don't know how to right unit test on "labelMatchedExpression" func. If you can help me on this ? |
Also I keep default "selected" value to true, because that broke matchLabels func. |
file-checks : End-To-End : |
Hi, |
As I mentioned before, I think we should rather focus on unit-tests, not e2e tests. The functionality you're working on is implemented through a separate function that doesn't depend on API-server. To get an idea on how unit-tests can be written, you can simply explore the existing code base. We heavily rely on Testify (specifically, |
@Ronan-WeScale Thanks, I'll try to take a look at this later this week. |
Hello, |
@andrejshapal you can help out to verify with your local setup and make sure that this works as intended. |
@Ronan-WeScale Finally, got some updates for you :) As you previously requested some help around writing/simplifying the unit-tests, here you go: Let's start with basic recommendations:
To make it easier to understand what exactly I'm referring to, I've rewritten some code and pushed to your branch (I hope you don't mind :)). I would suggest you to just go through commits to see how the code was changing along the way. With those changes, I think the code should be more or less ready to be merged, but I would like you, fellow maintainers, and any community members to go through the code to see if something is off. P.S. Normally, it's easier to have your code merged to our code base, you just happened to take relatively difficult task that affects base functionality, so it requires more effort / precision :) |
@andrejshapal could you run your tests again against the latest code?
That's interesting. I think it's actually a bug that is not related to this PR, but that is actually present in other builds as well. Thanks for reporting that! It's now tracked under #1519 |
All good Test three (grafana-operator fix-1401 commit 3827260):
Test result: Passed 🟩 |
Hi @weisdd, thank for all this informations and refactoring, like I said previously, I'm not developper and try to do my best in golang ( working and dirty :p ) but I prefer work or propose a solution, instead pointing a problem and go away. I will try to understand you're simplification of the unit tests, and maybe propose some unit-tests that's not already righted :) PS: sorry for my poor english |
Well, proposing a solution is often the most difficult part, so it doesn't matter that much if something gets merged as is or not. And I'm not a professional developer either. In fact, my first contribution here was actually my very first code written Go :) As for removal of the e2e-test, it's simply not needed here. - Everything is already covered with unit-tests including the test case from @andrejshapal (I asked him to double-check just to make sure I haven't overlooked anything). |
Fix #1401
Tested on multiple dashboard, used cases are in PR and I tested with my personal use case too :