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
Add CONTRIBUTING.md to sig scheduling #7292
Add CONTRIBUTING.md to sig scheduling #7292
Conversation
Signed-off-by: kerthcet <kerthcet@gmail.com>
/sig scheduling |
/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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
+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>
Updated. PTAL. |
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
/label 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 |
Since it's approved, so I'd like to unhold this PR. |
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