Skip to content
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

Merged

Conversation

Szymongib
Copy link
Contributor

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

)

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>
@Szymongib Szymongib requested a review from a team as a code owner March 23, 2021 17:56
Signed-off-by: Szymon Gibała <szymongib@gmail.com>
Copy link
Member

@alpeb alpeb left a 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 😉

Copy link
Contributor

@dadjeibaah dadjeibaah left a 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.

@Szymongib
Copy link
Contributor Author

Thanks a lot for review @alpeb and @dadjeibaah!

@alpeb a questions regarding your comment

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 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?

@alpeb
Copy link
Member

alpeb commented Mar 31, 2021

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.

@olix0r olix0r added this to the stable-2.11 milestone Apr 1, 2021
… messages in tests

Signed-off-by: Szymon Gibała <szymongib@gmail.com>
Signed-off-by: Szymon Gibała <szymongib@gmail.com>
@Szymongib
Copy link
Contributor Author

I have adjusted PR to address your comments, let me know if you have any other concerns or see ways to improve.

Changes:

  • Move labels and annotations checks to healthcheck_labels.go file
  • Split services check to two to align error messages with pods check - one check for labels the other for annotations
  • Change error messages according to suggestions
  • Check if error message is correct in tests

@Szymongib Szymongib requested review from alpeb and dadjeibaah April 6, 2021 17:33
Signed-off-by: Szymon Gibała <szymongib@gmail.com>
Signed-off-by: Szymon Gibała <szymongib@gmail.com>
Copy link
Contributor

@dadjeibaah dadjeibaah left a 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>
@Szymongib Szymongib requested a review from dadjeibaah April 14, 2021 13:29
Copy link
Member

@alpeb alpeb left a 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! 👍

Copy link
Contributor

@Pothulapati Pothulapati left a 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?

@alpeb alpeb merged commit c06208e into linkerd:main Apr 16, 2021
@Szymongib Szymongib deleted the feat/validate-labels-and-annotation-on-check branch April 17, 2021 06:37
Szymongib added a commit to Szymongib/website-1 that referenced this pull request Apr 17, 2021
Related PR linkerd/linkerd2#5943

Related Issue linkerd/linkerd2#5718

Signed-off-by: Szymon Gibała <szymongib@gmail.com>
@Szymongib
Copy link
Contributor Author

Hi @Pothulapati, sure!
I have created the PR linkerd/website#1046 let me know if that is correct place and if description is sufficient.

cpretzer pushed a commit to linkerd/website that referenced this pull request Apr 20, 2021
* 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>
jijeesh pushed a commit to jijeesh/linkerd2 that referenced this pull request Apr 21, 2021
)

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check should warn when expected labels and annotations are confused
5 participants