-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 check verifying labels and annotation are not confused (#5718) #5943
Add check verifying labels and annotation are not confused (#5718) #5943
Conversation
) No indication that labels and annotations were set incorrectly. Check if labels set by user are not set as annotations and vice versa. Unit tests and manual test. Fixes linkerd#5718 Signed-off-by: Szymon Gibała <szymongib@gmail.com>
Signed-off-by: Szymon Gibała <szymongib@gmail.com>
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.
Looking good 👍
What do you think about moving the functions in healthcheck.go
to a new file like healthcheck_labels.go
so we keep this file size under control?
Also it'd be great to add a test case into healthcheck_test.go
with a couple of resources violating these checks so we can ensure the output of the checks is right 😉
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.
@Szymongib, thanks for putting this PR together! This is going to be super useful. I think the change is looking great. I left a few suggestions for your consideration.
Thanks a lot for review @alpeb and @dadjeibaah! @alpeb a questions regarding your comment
There are already some tests violating the check that expect the error https://github.com/linkerd/linkerd2/pull/5943/files#diff-932ac70a02fcd5e0e4b89b0a03f1302aeac2d162ea1b7ae7a074839d24e6dbf5R1921 do you suggest to add assertion of the actual error message? |
Ah sorry I missed that, but yeah, it'd be nice to check the errors themselves as we do in the other negative tests in that file. |
… messages in tests Signed-off-by: Szymon Gibała <szymongib@gmail.com>
Signed-off-by: Szymon Gibała <szymongib@gmail.com>
I have adjusted PR to address your comments, let me know if you have any other concerns or see ways to improve. Changes:
|
Signed-off-by: Szymon Gibała <szymongib@gmail.com>
Signed-off-by: Szymon Gibała <szymongib@gmail.com>
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 looks great! Thanks for making those updates. I had one minor nit but that's pretty much it
Signed-off-by: Szymon Gibała <szymongib@gmail.com>
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.
Thanks @Szymongib , looks great! 👍
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 taking this up! 🔥 @Szymongib
Can you also update the linkerd/website
repo with the troubleshooting docs for the newly added hint anchors? They could just mention an explanation on what labels and annotations the CLI expects for pods and services?
Related PR linkerd/linkerd2#5943 Related Issue linkerd/linkerd2#5718 Signed-off-by: Szymon Gibała <szymongib@gmail.com>
Hi @Pothulapati, sure! |
* Document new checks added as part of #5943 Related PR linkerd/linkerd2#5943 Related Issue linkerd/linkerd2#5718 Signed-off-by: Szymon Gibała <szymongib@gmail.com>
) No indication that labels and annotations were set incorrectly. Check if labels set by user are not set as annotations and vice versa. Fixes linkerd#5718 Signed-off-by: Szymon Gibała <szymongib@gmail.com> Signed-off-by: Jijeesh <jijeesh.ka@gmail.com>
No indication that labels and annotations were set incorrectly.
Check if labels set by user are not set as annotations and vice versa.
Unit tests and manual test.
Fixes #5718
Signed-off-by: Szymon Gibała szymongib@gmail.com