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

e2e-tests.md: update description of labels #7824

Merged
merged 2 commits into from
Jun 17, 2024

Conversation

pohly
Copy link
Contributor

@pohly pohly commented Apr 17, 2024

This explains how to use --filter-label (possible since Kubernetes 1.29) and clarifies the usage of feature gate labels.

The intent is that adding tests with only a FeatureGate label is sufficient. Before we allow such tests, we have to

That is because the "traditional" SKIP=\[Features:.+\] will no longer match tests which have [FeatureGate:Foo] [Alpha], as explained in kubernetes/kubernetes#124350.

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 17, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added area/developer-guide Issues or PRs related to the developer guide sig/testing Categorizes an issue or PR as relevant to SIG Testing. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 17, 2024

# Run tests in parallel, skip any that must be run serially and keep the test namespace if test failed
GINKGO_PARALLEL=y kubetest --test --test_args="--ginkgo.skip=\[Serial\] --delete-namespace-on-failure=false"
GINKGO_PARALLEL=y kubetest --test --test_args='--ginkgo.label-filter=!Serial --delete-namespace-on-failure=false'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

!Serial is special in an interactive bash when included in double quotes. Therefore I switched to single quotes here and (for the sake of consistency) also above.

@aojea
Copy link
Member

aojea commented Apr 23, 2024

/lgtm

/assign @danwinship @BenTheElder @SergeyKanzhelev

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 23, 2024
@pohly
Copy link
Contributor Author

pohly commented May 3, 2024

/hold

The intent is that adding tests with only a FeatureGate label is sufficient. Before we allow such tests, we have to do some preparations - see checklist in description.

@k8s-ci-robot k8s-ci-robot added sig/contributor-experience Categorizes an issue or PR as relevant to SIG Contributor Experience. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jun 11, 2024
@aojea
Copy link
Member

aojea commented Jun 11, 2024

do we want to merge and do the modifications or do the modifications and then merge based on the feedback?

@aojea
Copy link
Member

aojea commented Jun 11, 2024

/lgtm

this brings a lot of progress

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 11, 2024
This explains how to use --filter-label (possible since Kubernetes 1.29) and
clarifies the usage of feature gate labels.
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 12, 2024
@pohly
Copy link
Contributor Author

pohly commented Jun 12, 2024

do we want to merge and do the modifications or do the modifications and then merge based on the feedback?

As the text stands right now, it says (or implies?) that "annotating a test which depends only on a feature gate with only FeatureGate is okay". That is not true yet because most of our jobs would then run that test.

We could merge it now if we added a comment saying that this is the goal but that "as a stop-gap solution, tests which depend on a feature gate also need to be annotated with a feature". It's probably worth explaining that and merge, because then the rest of the new documentation, in particular the part about label filtering, is what people will see when they check the master branch. Agreed?

While we now have the ability to support tests properly which only depend on
alpha or beta feature gates, we still haven't migrated all jobs. So for now,
developers still need to follow the old practice of tagging such tests with
both [Feature] and [FeatureGate].
@pohly
Copy link
Contributor Author

pohly commented Jun 13, 2024

@aojea: I added such an explanation to the Alpha/Beta section. This is therefore ready for merging.

@pohly
Copy link
Contributor Author

pohly commented Jun 13, 2024

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 13, 2024
@aojea
Copy link
Member

aojea commented Jun 17, 2024

/lgtm

lazy consensus

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 17, 2024
@k8s-ci-robot k8s-ci-robot merged commit 4a6113a into kubernetes:master Jun 17, 2024
3 checks passed
@pohly
Copy link
Contributor Author

pohly commented Jun 17, 2024

Sorry, I had already lifted the hold because the reason there no longer applied. I hadn't considered that I self-approve in this repo.

Let's give @danwinship @BenTheElder @SergeyKanzhelev time until Wednesday to comment and suggest further changes before I send a mail to [email protected] about this.

@aojea
Copy link
Member

aojea commented Jun 17, 2024

Let's give @danwinship @BenTheElder @SergeyKanzhelev time until Wednesday to comment and suggest further changes before I send a mail to [email protected] about this.

it got merged :) ... we can always amend changes or modify, is not that this is set in stone

@pohly
Copy link
Contributor Author

pohly commented Jun 17, 2024

Yes, that's what I meant, too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/developer-guide Issues or PRs related to the developer guide cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/contributor-experience Categorizes an issue or PR as relevant to SIG Contributor Experience. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants