Skip to content
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

Added ttl validation in admission controller #167

Merged
merged 1 commit into from May 15, 2019

Conversation

SrinivasChilveri
Copy link
Contributor

ttl validation in admission controller.

travis result

VolcanoTravis_ 2019-05-15 11-45-14

@SrinivasChilveri
Copy link
Contributor Author

@hzxuzhonghu ,added validation in admission controller. pls do have a look.

@hzxuzhonghu
Copy link
Collaborator

/lgtm

@SrinivasChilveri Could you add a default value for it when it is nil?

@volcano-sh-bot volcano-sh-bot added the lgtm Indicates that a PR is ready to be merged. label May 15, 2019
@TommyLike
Copy link
Contributor

/lgtm

@SrinivasChilveri Could you add a default value for it when it is nil?

Would be conflicted to your design:

	// ttlSecondsAfterFinished limits the lifetime of a Job that has finished
	// execution (either Completed or Failed). If this field is set,
	// ttlSecondsAfterFinished after the Job finishes, it is eligible to be
	// automatically deleted. If this field is unset,
	// the Job won't be automatically deleted. If this field is set to zero,
	// the Job becomes eligible to be deleted immediately after it finishes.
	// +optional

/lgtm

@k82cn
Copy link
Member

k82cn commented May 15, 2019

/lgtm
/approve

@volcano-sh-bot volcano-sh-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 15, 2019
@volcano-sh-bot volcano-sh-bot merged commit fd4e5a0 into volcano-sh:master May 15, 2019
@SrinivasChilveri
Copy link
Contributor Author

/lgtm

@SrinivasChilveri Could you add a default value for it when it is nil?

can i add default value as 10 ?

@hzxuzhonghu
Copy link
Collaborator

@TommyLike Yeah, should update the desc.

can i add default value as 10 ?

I think this should be large enough to allow users to check logs / results. I prefer 1 day, @k82cn @TommyLike WDYT

kevin-wangzefeng pushed a commit to kevin-wangzefeng/volcano that referenced this pull request Jun 28, 2019
Added ttl validation in admission controller
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants