-
Notifications
You must be signed in to change notification settings - Fork 231
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
Skip failing anti-affinity rules if at least one valid rule is found. #373
Conversation
…rs if no anti-affinity rule could be validated.
} | ||
foundIssues = append(foundIssues, diagnostic.Diagnostic{ | ||
Message: err.Error(), | ||
}) | ||
} | ||
return foundIssues |
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 will return nil
if no issues are found, which is wrong. (Which also suggests we need some unit tests here 😅)
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.
Hm, but what would the expected behavior be if we didn't find any issues?
We are already short-circuiting if no anti-affinity rule is created beforehand.
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.
Ah, just saw that short-circuit. Well, we need to enhance it so that it also handles the case where podAntiAffinity is not nil
, but both the arrays ("required" and "preferred") are empty. If we short-circuit in that case as well, then we should be good.
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.
See comment
Dismissing since we got an approval from Tomek. If there's anything else required, we can create another PR for it.
Description
When adding more verbose errors to no-anti-affinity check, we remove the short-circuiting on the first valid anti-affinity rule.
However, we should short-circuit on the first anti-affinity rule, as this is the expected behavior of the check.
Fixes #369