-
Notifications
You must be signed in to change notification settings - Fork 459
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
Clean up code for custom CRD #1355
Clean up code for custom CRD #1355
Conversation
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
178c775
to
364df94
Compare
/retest |
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
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andreyvelich, 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 |
@andreyvelich: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
364df94
to
00e1d9b
Compare
New changes are detected. LGTM label has been removed. |
I removed redundant code and fixed tests for new custom CRD implementation.
As well, I added some default values for
Job
,PyTorchJob
andTFJob
to make examples easier for the first look, as we discussed before.In particular, for
Job
:SuccessCondition
andFailureCondition
.I didn't add
PrimaryPodLabels
toJob
examples since eachJob
has only one pod replica. IfPrimaryPodLabels
is omitted, metrics collector injected to all created pods.For Kubeflow Job (
PytorchJob
andTFJob
).SuccessCondition
andFailureCondition
.PrimaryPodLabels
.I manually added
PrimaryContainerName
to all examples. I think it is very easy to understand and user can explicitly see which container will be wrapped by metrics collector.I can't set default values if Trial Template is set in ConfigMap, since it requires k8s client. We can think about it later.
I left validation for
Job
,TFJob
andPyTorchJob
for now. @gaocegege @johnugeorge What do you think about it ?I deleted
job/v1beta1
since it is not used. Let's continue discussion here: #1320 how we should implementMutateJob
./assign @gaocegege @johnugeorge