-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Trim e2e skip regexes for Cilium #15753
Conversation
/test pull-kops-e2e-cni-cilium |
The kops master branch is used to build kubetest2-kops for all e2e jobs including upgrade tests that upgrade to older kops versions that still use cilium 1.12. This change will stop skipping those tests for cilium 1.12 clusters which will cause failures. Maybe we could update build_jobs.py to clone the release branch for upgrade tests that upgrade to non-latest kops versions? |
/test pull-kops-e2e-cni-cilium |
K8s 1.28 will be released a week from now. I think what @rifelpet was doing with checking K8s version instead of kOps version was pretty good. We already do checks against |
if err != nil { | ||
return err | ||
} | ||
isPre28 := kopsVersion < "1.28" |
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.
nit: This will evaluate to false for any future 1.28 pre-release builds
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.
And we can't inspect the Cluster manifest because spec.networking.cilium.version
isn't guaranteed to be set. I suppose one alternative would be to inspect the daemonset itself with the kubectl binary that the tester downloads:
kops/tests/e2e/pkg/tester/kubectl.go
Lines 132 to 134 in 7e06e4c
func KubectlPath() string { | |
return artifacts.RunDir() + "/kubectl" | |
} |
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.
Evaluating false for future 1.28 pre release builds is desired behavior
Checking k8s version has the undesirable behavior of skipping those tests on kops 1.28+ branch kops and <1.28 k8s |
In my opinion, that is irrelevant. We aim for most maintainable code here, not for exact skip list. |
Being able to remove the extra skips earlier leads to more maintainable code. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rifelpet The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Closes #15448