-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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: tag documentation + sorting #123260
e2e: tag documentation + sorting #123260
Conversation
Adding new entries at the bottom is tempting, but increases the risk of merge conflicts between unrelated PRs. Let's use alphabetic order instead.
Adding a doc comment for all existing items makes it more obvious that new items should be documented more carefully. It also has the welcome side effect that each item gets indented independently from the others.
This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
// The test does not work in UserNS (for example, `open /proc/sys/kernel/shm_rmid_forced: permission denied`). | ||
NotInUserNS = framework.WithEnvironment(framework.ValidEnvironments.Add("NotInUserNS")) | ||
|
||
// Please keep the list in alphabetical order. |
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.
This comment at the bottom doesn't make a lot of sense here, but it is relevant for the larger lists: if it had been present earlier, then the diff in 6073d1c#diff-337683b7f1560f35fe4a306d97241505a4b6e852d0aea485221d7f6ab67524d8 would have shown the request to keep the list sorted.
Adding that comment only at the top is not enough for that.
/assign @aojea |
NoSNAT = framework.WithFeature(framework.ValidFeatures.Add("NoSNAT")) | ||
// Please keep the list in alphabetical order. | ||
|
||
// TODO: document the feature (owning SIG, when to use this feature for a test) |
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.
Documenting "when to use this feature" has two purposes:
- making it clear when to reuse an existing label for a new test
- together with the owning SIG, make it clear which jobs should test the feature and how it needs to be configured
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.
The "how it needs to be configured" might have to be called out explicitly?
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.
/lgtm
LGTM label has been added. Git tree hash: 6181bcb3f188da546ea971d97a0c5d1703aa5e03
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pohly, SergeyKanzhelev 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 |
The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass. This bot retests PRs for certain kubernetes repos according to the following rules:
You can:
/retest |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Adding new entries at the bottom is tempting, but increases the risk of merge
conflicts between unrelated PRs. Let's use alphabetic order instead.
Adding a doc comment for all existing items makes it more obvious that new
items should be documented more carefully. It also has the welcome side effect
that each item gets indented independently from the others.
Which issue(s) this PR fixes:
Related-to:
Does this PR introduce a user-facing change?