Add container lifecycle hooks in CDAPServiceSpec#79
Merged
Conversation
ghost
suggested changes
Jan 13, 2022
Author
|
Thanks for the review @arjansbal . Updated the test-case. |
dli357
requested changes
Jan 13, 2022
api/v1alpha1/cdapmaster_types.go
Outdated
| SecretVolumes map[string]string `json:"secretVolumes,omitempty"` | ||
| // SecurityContext overrides the security context for the service pods. | ||
| SecurityContext *SecurityContext `json:"securityContext,omitempty"` | ||
| // Lifecycle is to specify Container Lifecycle hooks provided by Kubernetes for containers |
Contributor
There was a problem hiding this comment.
Please add a comment specifying how these lifecycle hooks are applied to containers, e.g. are they applied to all containers (including init containers)? Or only specific containers?
templates/cdap-deployment.yaml
Outdated
| {{if $c.ImagePullPolicy}} | ||
| imagePullPolicy: {{$c.ImagePullPolicy}} | ||
| {{end}} | ||
| {{if $c.Lifecycle}} |
Contributor
There was a problem hiding this comment.
Can we directly serialize the Lifecycle object instead of manually copying individual components? Otherwise you end up with a lot of nested if statements. For an example, please see #73, specifically deployment.go#341 and deployment.go#372.
Author
There was a problem hiding this comment.
Thanks for the suggestion. Updated.
templates/cdap-sts.yaml
Outdated
| {{if $c.ImagePullPolicy}} | ||
| imagePullPolicy: {{$c.ImagePullPolicy}} | ||
| {{end}} | ||
| {{if $c.Lifecycle}} |
rahul1995
force-pushed
the
fix/CDAP-18736-lifecycle-hook
branch
3 times, most recently
from
94cfb05 to
f3d9c78
Compare
rahul1995
requested review from
a user and
dli357
and removed request for
dli357
dli357
reviewed
Jan 19, 2022
templates/cdap-deployment.yaml
Outdated
| # NOTE: Some properties are highly nested and it is not feasible to pick them from the template file | ||
| # hence those are added directly in the code. | ||
| # For example, please check the method "../controllers/deployment.go#buildDeploymentObject" and see | ||
| # how container's lifecycle property is added to the k8s object |
dli357
reviewed
Jan 19, 2022
templates/cdap-sts.yaml
Outdated
| # NOTE: Some properties are highly nested and it is not feasible to pick them from the template file | ||
| # hence those are added directly in the code. | ||
| # For example, please check the method "../controllers/deployment.go#buildStatefulSetsObject" and see | ||
| # how container's lifecycle property is added to the k8s object |
dli357
approved these changes
Jan 19, 2022
Contributor
dli357
left a comment
dli357
left a comment
There was a problem hiding this comment.
LGTM, please don't forget to rebase. Thanks!
rahul1995
force-pushed
the
fix/CDAP-18736-lifecycle-hook
branch
from
f3d9c78 to
39c8910
Compare
…lot of nested fields would be needed in the template
rahul1995
force-pushed
the
fix/CDAP-18736-lifecycle-hook
branch
from
39c8910 to
5423919
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.