migrate CRD TrainingJob structure #12
Conversation
|
CI issue is caused by pre-commit, see #10 , merge that change will get this fixed. |
m3ngyang
force-pushed
the
trainingjob-strcut
branch
from
f2ad548
to
6bac249
Compare
|
Conflicts fixed. |
| } | ||
|
|
||
| // addKnownTypes adds the set of types defined in this package to the supplied scheme. | ||
| func addKnownTypes(scheme *runtime.Scheme) error { |
There was a problem hiding this comment.
Maybe don't return error if it's not possible to have error. Otherwise the caller need to unnecessarily handle it.
There was a problem hiding this comment.
Because the parameter of the caller function NewSchemeBuilder is funcs ...func(*Scheme) error, we must return nil even if there is no error.
|
|
||
| // CRDName returns name of crd | ||
| func CRDName() string { | ||
| return fmt.Sprintf("%s.%s", CRDKindPlural, CRDGroup) |
There was a problem hiding this comment.
Maybe just return CRDKindPlural + "." + CRDGroup, which will be optimized to a compile time constant. Anyway, this is very minor, fmt.Sprintf() is very fast anyway.
|
|
||
| const ( | ||
| // MASTER is the master name of TrainingResourceType. | ||
| MASTER TrainingResourceType = "MASTER" |
There was a problem hiding this comment.
Is it a Kubernetes convention to have it named all capital case (MASTER vs Master). The Go convention is camel case (Master).
There was a problem hiding this comment.
I'll change the format in the next pr.
| // This package is generated by client-gen with custom arguments. | ||
|
|
||
| // Package fake has the automatically generated clients. | ||
| package fake |
There was a problem hiding this comment.
Just curious what does "fake" package do? Is it something generated and used by Kubernetes?
There was a problem hiding this comment.
The packages under client are auto-generated by the official tool k8s.io/code-generator, and fake clientset that can be used in tests.
| return informers | ||
| }() | ||
|
|
||
| res := map[reflect.Type]bool{} |
There was a problem hiding this comment.
Is f.lock.Lock() required here?
| Paddlepaddle() paddlepaddle.Interface | ||
| } | ||
|
|
||
| func (f *sharedInformerFactory) Paddlepaddle() paddlepaddle.Interface { |
There was a problem hiding this comment.
This method is a little strange, typically developer calls paddlepaddle.New(f), rather than f.Paddlepaddle(). Is it a Kubernetes convention?
This pr comes from PaddlePaddle/PaddleCloud#642 , including two parts: