-
Notifications
You must be signed in to change notification settings - Fork 17
Conversation
Thanks for your invitation. I am glad to help you to review the code. |
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.
Generally LGTM while request @rongou 's review.
@@ -0,0 +1,13 @@ | |||
required = ["k8s.io/code-generator/cmd/client-gen"] |
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.
I am not sure if we only use client-gen. Then how about defaulter-gen/informer-gen and so on?
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.
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. "k8s.io/code-generator/cmd/client-gen" comes with deepcopy/client/defaulter/lister/informer-gen.
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.
Then LGTM
deploy/0-crd.yaml
Outdated
- chjobs | ||
categories: | ||
- all | ||
|
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.
Please remove the blanks.
slots = Int32(int32(q.Value())) | ||
} | ||
spec.MPIConfig = defaultMPIConfig(slots) | ||
glog.V(4).Infof("setting spec.WorkerSets[%+v].MPIConfig.Slots to %+v", name, *spec.MPIConfig.Slots) |
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.
I think we prefer logrus because of structured logging. But we could update it later.
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.
oh, ok. I will do it later. I opened an issue on it #14
// Kubernetes API. | ||
recorder record.EventRecorder | ||
|
||
backends map[apisv1alpha1.BackendType]backends.Backend |
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.
Personally, I do not recommend maintaining such a cache in memory, and k8s community also does not recommend. It will be better to be event-driven. While LGTM since this is the first commit. We could merge it and go ahead iteratively.
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!! I will. I opened the issue on it #13
/lgtm @everpeace Feel free to approve your PR and the tide will help you to merge it. Thanks for your contribution! |
Thanks! I'll approve this PR. /approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: everpeace 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 |
This is my initial implementation of chainer-operator. My proposal is here
Notes:
@rongou @gaocegege @jlewi Would you mind if I asked you to be reviewers?? If you don't mind, I would like to add you guys to
OWNERS
file as reviewers. I will also seek volunteer reviewers in slack channel too like rongou did before.