-
Notifications
You must be signed in to change notification settings - Fork 716
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 ValidateAlphaTwoTFJobSpec to check v1alpha2.TFJobSpec is valid #702
add ValidateAlphaTwoTFJobSpec to check v1alpha2.TFJobSpec is valid #702
Conversation
/ok-to-test |
Travis tests have failedHey @codeflitting, 2nd Buildgometalinter --config=linter_config.json --vendor ./...
3rd Buildgometalinter --config=linter_config.json --vendor ./...
|
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 add some test cases for ValidateAlpha2TFJobSpec
// ValidateTFJobSpec checks that the v1alpha2.TFJobSpec is valid. | ||
func ValidateAlpha2TFJobSpec(c *tfv2.TFJobSpec) error { | ||
for _, value := range c.TFReplicaSpecs { | ||
if &value.Template == nil || len(value.Template.Spec.Containers) == 0 { |
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.
No need to add & here.
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.
remove & will generate an error “Cannot contert nil to type v1.PodTemplateSpec”
pkg/controller.v2/informer.go
Outdated
@@ -92,7 +93,8 @@ func tfJobFromUnstructured(obj interface{}) (*tfv1alpha2.TFJob, error) { | |||
// This is a simple validation for TFJob to close | |||
// https://github.com/kubeflow/tf-operator/issues/641 | |||
// TODO(gaocegege): Add more validation here. | |||
if err != nil || tfjob.Spec.TFReplicaSpecs == nil { | |||
validationErr := validation.ValidateAlpha2TFJobSpec(&tfjob.Spec) | |||
if err != nil || tfjob.Spec.TFReplicaSpecs == nil || validationErr != nil { |
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 move the check tfjob.Spec.TFReplicaSpecs == nil
to the function ValidateAlpha2TFJobSpec
return fmt.Errorf("TFJobSpec is not valid") | ||
} | ||
for _, value := range c.TFReplicaSpecs { | ||
if len(value.Template.Spec.Containers) == 0 { |
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 please add the &value check here.
&value.Template == nil
Travis tests have failedHey @codeflitting, 2nd Buildgometalinter --config=linter_config.json --vendor ./...
3rd Buildgometalinter --config=linter_config.json --vendor ./...
|
Travis tests have failedHey @codeflitting, 2nd Buildgometalinter --config=linter_config.json --vendor ./...
3rd Buildgometalinter --config=linter_config.json --vendor ./...
|
Travis tests have failedHey @codeflitting, 2nd Buildgometalinter --config=linter_config.json --vendor ./...
3rd Buildgometalinter --config=linter_config.json --vendor ./...
|
/retest |
Travis tests have failedHey @codeflitting, 1st Buildgometalinter --config=linter_config.json --vendor ./...
3rd Buildgometalinter --config=linter_config.json --vendor ./...
|
If we can not finish the test, we can add it into ignore files. |
Signed-off-by: Pengyu Chen <[email protected]>
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gaocegege 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 |
…ubeflow#702) Signed-off-by: Pengyu Chen <[email protected]>
add ValidateAlphaTwoTFJobSpec to check v1alpha2.TFJobSpec is valid #682
/cc @gaocegege
Signed-off-by: Pengyu Chen [email protected]
This change is