Make implicitly dependent feature explicitly depend on each other #1551
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
By running a variant of cargo hack, we found a number of compile issues when using unusual feature combinations, and these are largely rectified by making intended feature pairings explicit. Client features now depend on the `client` feature, runtime features now depend on the `runtime` feature, etc. - rustls-tls / openssl-tls => `client` feature - oauth / oidc / gzip / socks5 / http-proxy => `client` feature - unstable-runtime => `runtime` feature Unfortunately, the cargo hack solution is simplistic and cannot verify everything AFAIKT yet. I am currently running with excluding oauth and oidc because these require picking a tls stack and we have an intended compile error for this. I cannot find a way to tell `hack` to model this without just excluding them. The number of features makes the combination features test very long, case in point, the cargo hack invocation in the justfile: - tests **11370** feature combinations - takes >2h (currently 30m in at 2000 combination on a beefy 7950X3D) Signed-off-by: clux <sszynrae@gmail.com>
clux
added
changelog-fix
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1551 +/- ##
=====================================
Coverage 75.7% 75.7%
=====================================
Files 79 79
Lines 7252 7252
=====================================
Hits 5487 5487
Misses 1765 1765 |
Signed-off-by: clux <sszynrae@gmail.com>
Signed-off-by: clux <sszynrae@gmail.com>
Signed-off-by: clux <sszynrae@gmail.com>
nightkr
approved these changes
Jul 30, 2024
# for free
to join this conversation on GitHub.
Already have an account?
# to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
By running an invocation of cargo hack, we found a number of compile issues when using unusual feature combinations, and these are largely rectified by making intended feature pairings explicit.
New Feature Dependencies
rustls-tls+openssl-tlsnow depend onclientoauth+oidc+gzip+socks5+http-proxy+unstable-clientnow depend onclientunstable-runtimenow depend onruntimeTesting Feasibility / cargo hack on CI
Because each feature basically doubles the amount of tests, we do actually need to restrict the input space from the raw
powerseta bit. Without restrictions (skippingoidc+oauthmandatory), we get:11370feature combinations>1hduration locally (my CPU sits mostly sitting idle in context switches)>60Gof artifacts intargetHowever, with grouping of commonly additive features, we can basically halve the number of tests per grouping. A moderately aggressive grouping of small client features (
socks5+http-proxy+gzip), smaller core features (admission+jsonpatch+derive) and client features (client+kubelet-debug) we get:624feature combinations5mduration locally on a blank cache / 8m duration on GHA with cache6Gof artifacts intargetSo, kind of works on CI. Have included it as a test on
mainonly (and this branch explicitly as a PoC).Limitations
kube.oauth/oidc:: should depend on a tls stack but we cannot make that choiceWe do have fallback
compile_errors for oidc and oauth, but I have not found a way to tellhackto model this without just excluding them.