Add CONTRIBUTING.md to sig scheduling #7292
Conversation
Signed-off-by: kerthcet <kerthcet@gmail.com>
k8s-ci-robot
added
the
cncf-cla: yes
|
/sig scheduling |
k8s-ci-robot
added
sig/scheduling
|
/hold |
k8s-ci-robot
added
the
do-not-merge/hold
|
Should we mention that each PR(exclude a simple one) should be reviewed by two approvers？ |
|
/assign |
There was a problem hiding this comment.
Thanks @kerthcet for having a draft!
I feel we need to cover the common technical mistakes/review comments. Like:
- Unless the change is minor, new changes probably need to have UT.
- Check/share a scheduler_perf result if the change may introduce degradation in a specific path.
- use cmp.Diff instead of reflect.Equal.
- framework.NewStatus(framework.Success) can be just replaced by nil.
- Use PodWrapper to build Pod in tests.
- how to decide log level.
- I'm also not sure about it to be honest. If there is a good doc from sig-instrumentation, then it's probably enough to just have a link for it.
sig-scheduling/CONTRIBUTING.md
Outdated
| * [first-good-issue](https://github.com/kubernetes/kubernetes/issues?q=is%3Aissue+is%3Aopen+label%3A%22good+first+issue%22+label%3Asig%2Fscheduling+) | ||
| * [help wanted](https://github.com/kubernetes/kubernetes/issues?q=is%3Aissue+is%3Aopen+label%3A%22help+wanted%22+label%3Asig%2Fscheduling+) | ||
|
|
||
| * If you want to understand the architectural details of scheduler, you can refer to the [design docs](https://github.com/kubernetes/community/tree/master/contributors/devel/sig-scheduling) |
There was a problem hiding this comment.
Actually, there are pretty old docs only.
Instead of (or in addition to) them, I'd prefer to have a link for some official doc explaining the scheduler like:
https://kubernetes.io/docs/concepts/scheduling-eviction/scheduling-framework/
There was a problem hiding this comment.
Actually, there are pretty old docs only.
We should update them.
I will mention the scheduling-eviction as well. Thanks.
Seems some small tips, we can have a bunch of that suggestions, maybe a new place? Glad to hear other's advices.
You can refer to https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/logging.md#logging-conventions from some guidances. |
|
cc @kubernetes/sig-scheduling-misc |
Signed-off-by: kerthcet <kerthcet@gmail.com>
|
Yep, having them on somewhere new also sounds good. |
It's rather the opposite. Most PRs require only one approver. Bigger PRs, or PRs that touch critical pieces (the framework, event handlers) might benefit from more than one approver, but not necessarily. In general, the approver would judge whether they want to involve another approver. |
It can be added as a follow up with a separate section :) /lgtm |
k8s-ci-robot
added
the
lgtm
+1 it's an important area to cover. But it's best to be constructed as separate doc(s) in contributors/devel/sig-scheduling, and linked back here. |
|
Tracked here #7296. |
Signed-off-by: kerthcet <kerthcet@gmail.com>
k8s-ci-robot
removed
the
lgtm
|
Updated. PTAL. |
k8s-ci-robot
added
tide/merge-method-squash
|
Leaving the approval to others |
|
cc @Huang-Wei @ahg-g |
|
/approve Thanks @kerthcet ! |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Huang-Wei, kerthcet 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 |
k8s-ci-robot
added
the
approved
|
Since it's approved, so I'd like to unhold this PR. |
k8s-ci-robot
removed
the
do-not-merge/hold
|
can you send an email to the mailing list about this published doc? |
|
/lgtm Thanks for applying the suggestions. |
I suppose you mean the sig scheduling mailing list. |
|
Yes :) |
Which issue(s) this PR fixes:
Fixes #7228