-
Notifications
You must be signed in to change notification settings - Fork 327
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
fix(kuma-cp) test versions compatibility on the release #2707
Conversation
Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
Codecov Report
@@ Coverage Diff @@
## master #2707 +/- ##
==========================================
+ Coverage 51.93% 51.98% +0.04%
==========================================
Files 871 872 +1
Lines 49530 49553 +23
==========================================
+ Hits 25725 25759 +34
+ Misses 21724 21704 -20
- Partials 2081 2090 +9
Continue to review full report at Codecov.
|
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.
Mainly some nits on naming. Component versioning tends to be not very obvious, so I'd really recommend adding comments with some Kuma history and context for the benefit of future maintainers :)
Entry("1.3.0", testCase{ | ||
dpVersion: "1.3.0", | ||
expectedEnvoyConstraint: "~1.18.4", | ||
}), |
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.
I'm not sure that we need all these test cases. We don't need to verify the exact compatibility data here, we need to verify the behavior of the API by ensuring we cover all the code paths, including error cases.
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.
Yeah, I wasn't sure about that either but I carred the tests from the previous code location. Let me remove excessive tests
pkg/version/compatibility.go
Outdated
}, | ||
} | ||
|
||
func (c Compatibility) DP(version string) (*KumaDPCompatibility, error) { |
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.
I think we can do better with the function name. This is getting the Dataplane compatibility record for the given Kuma release, right? Perhaps DataplaneConstraints
could work?
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.
Followup ... since this API isn't used currently, do you think it is likely to be used in the future?
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.
It might be (for example in kumactl)
Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com> (cherry picked from commit 8ae816b)
Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
Summary
In Kuma 1.3.0, we once again made a mistake of not updating
/versions
endpoint so we see a warning in the GUI.This PR introduces unit test that is executed on the release that checks if the current tag has a semver section in
CompatibilityMatrix
.Full changelog
pkg/versions
. This information is too important to be squeezed into API WS package. This also helps with tests.Documentation
Testing
Backwards compatibility
backport-to-stable
label if the code is backwards compatible. Otherwise, list breaking changes.