-
Notifications
You must be signed in to change notification settings - Fork 710
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
crd: Add validation using OpenAPI 3.0 #605
Conversation
/retest |
If we could find the validation for pod template spec, then we could reuse it here. While I do not find it. |
/retest |
2 similar comments
/retest |
/retest |
I think the test failures could be kubeflow/kubeflow#869 |
examples/crd/crd-v1alpha2.yaml
Outdated
Worker: | ||
properties: | ||
replicas: | ||
type: integer |
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.
Why is the minimum 1? Shouldn't the minimum be 0 for Worker & PS since they aren't required?
Even chief should have minimum 0 because in some cases we will have N workers and the first worker will be the chief and there will be no explict chief.
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.
If there is no worker or PS, the check will not be executed. Thus this is for the case that we have the worker/ps/chief definition.
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.
Can we add a comment to that effect?
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. Can we put a comment to that effect?
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.
OK, I will do it.
@@ -9,3 +9,26 @@ spec: | |||
kind: TFJob |
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.
Why are you doing this here and not in the ksonnet package?
https://github.com/kubeflow/kubeflow/blob/master/kubeflow/core/tf-job-operator.libsonnet#L44
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 intented to get ack for the current changes and use tools to convert YAML to json. Then I will put the code to the ksonnet package. Sorry for that 😄
Signed-off-by: Ce Gao <[email protected]>
Signed-off-by: Ce Gao <[email protected]>
/assign @jlewi |
/hold /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jlewi 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 |
/hold cancel I will file a new PR if I find a way to check the podtemplatespec |
* crd: Add validation using OpenAPI 3.0 Signed-off-by: Ce Gao <[email protected]> * crd-v1alpha2: Add comment Signed-off-by: Ce Gao <[email protected]>
* crd: Add validation using OpenAPI 3.0 Signed-off-by: Ce Gao <[email protected]> * crd-v1alpha2: Add comment Signed-off-by: Ce Gao <[email protected]>
/assign @jlewi @ScorpioCPH
The feature is restricted and I only add validation for replicas. This avoids the crash problem when replicas < 0.
related to: #437
Signed-off-by: Ce Gao [email protected]
This change is