-
Notifications
You must be signed in to change notification settings - Fork 1.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
e2e-k8s.sh: support --ginkgo.label-filter #3582
Conversation
Is my understanding correct that this change will be usable right after merging, i.e. without a formal kind release? |
yes @pohly @onsi this output looks weird,
|
@aojea i don’t know if y’all also call |
We do a lot of runtime skipping. I think that explains it. |
hack/ci/e2e-k8s.sh
Outdated
FOCUS="${FOCUS:-"\\[Conformance\\]"}" | ||
FOCUS="${FOCUS:-}" | ||
LABEL_FILTER="${LABEL_FILTER:-}" | ||
if [ -z "${SKIP}" ] && [ -z "${FOCUS}" ] && [ -z "${LABEL_FILTER}" ]; then |
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.
SKIP was not influencing the defaulting of Focus before, to be backwards compatible should not this be ?
if [ -z "${SKIP}" ] && [ -z "${FOCUS}" ] && [ -z "${LABEL_FILTER}" ]; then | |
if [ -z "${FOCUS}" ] && [ -z "${LABEL_FILTER}" ]; then |
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.
Yes, I think you are right. Changed.
As you were asking about SKIP, I also looked at the surrounding lines a bit more and started to wonder whether we should still use FOCUS and SKIP at all as defaults. We could get the same effect with the new label filter.
I decided against it (again?) because it's better to be conservative and to keep using what we used before. It also minimizes the changes to the script.
But I could also be convinced to be more audacious and clean this up so that FOCUS and SKIP are only set when explicitly requested. That would expedite our transition to using labels. 🤷
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.
That would expedite our transition to using labels.
better do baby steps, so we can identify clearly what change do what
one last comment and we are good to go https://github.com/kubernetes-sigs/kind/pull/3582/files#r1635882841 |
The label filter query is more expressive (logical operations) and readable (no regexp unless absolutely required). Such a query can be combined with focus + skip, but in practice a single label filter can replace both of those and is easier to understand. Kubernetes has supported ginkgo v2 and thus --label-filter since v1.25.0. This makes it safe to pass that command line flag unconditionally when invoking the E2E suite.
ca4944d
to
165e08b
Compare
/lgtm /cc: @BenTheElder |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aojea, pohly 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 |
we should also think in phasing this: 1.32 phase out and forbid focus and skip in test-infra with a. linter |
The label filter query is more expressive (logical operations) and readable (no regexp unless absolutely required). Such a query can be combined with focus + skip, but in practice a single label filter can replace both of those and is easier to understand.
Kubernetes has supported ginkgo v2 and thus --label-filter since v1.25.0. This makes it safe to pass that command line flag unconditionally when invoking the E2E suite. However, actual labels were only added in Kubernetes 1.29.
Note: I have not tested this yet. If we are confident that it'll work, then we can merge and I'll start using it in a small job (most likely DRA) once it is available.