-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Merge *Run's PodTemplate with Default 🧙 #4057
Merge *Run's PodTemplate with Default 🧙 #4057
Conversation
This also probably needs some docs 🙃 |
/retest |
a3567a8
to
b6352d2
Compare
The following is the coverage report on the affected files.
|
/retest |
/cc @piyush-garg |
This looks good from my perspective. 👍 /approve |
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
@vdemeester you mentioned adding some docs, is this something we can do before we merge? (maybe at https://github.com/tektoncd/pipeline/blob/main/docs/podtemplates.md) Also looks like the coverage went down slightly - in both cases it looks like it's just one line per file not getting touched, maybe it's easy to add those to the test cases? 🙏 |
Right, now when you specify a default pod template and also specify pod template at pipelinerun/taskrun level. The default pod template is getting overridden by the one at the pipelinerun/taskrun level. This changes this behavior by only overriding the `PodTemplate` field that are defined by the `TaskRun` and not the whole `PodTemplate`. This means it is possible to define a `NodeSelector` widely (through the default) without it being overriden when the user sets a `SecurityContext` on his `TaskRun`. This is also done on the `PipelineRun` PodTemplate. Signed-off-by: Vincent Demeester <vdemeest@redhat.com>
b6352d2
to
131bab1
Compare
/hold cancel |
The following is the coverage report on the affected files.
|
Re-adding my approve. /approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sbwsg 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 |
switch { | ||
case defaultTpl == nil: | ||
// No configured default, just return the template | ||
return tpl |
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.
Could tpl
be nil
here? or not possible to have both defaultTpl
and tpl
to be nil
at this point?
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.
or don't bother to merge, just return tpl
here irrespective of it being 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.
Yeah if there is no default, we just return the taskrun template, be it nil or not 😛
default: | ||
// Otherwise, merge fields | ||
if tpl.NodeSelector == nil { | ||
tpl.NodeSelector = defaultTpl.NodeSelector |
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.
do we need a check here if defaultTpl.NodeSelector
is not nil
and likewise for the rest of the fields?
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.
or again just return default irrespective of it being nil
/lgtm thanks @vdemeester |
tests seems flaky /test tekton-pipeline-unit-tests |
/retest |
2 similar comments
/retest |
/retest |
} | ||
if tpl.HostNetwork == false && defaultTpl.HostNetwork == true { | ||
tpl.HostNetwork = true | ||
} |
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.
Maybe sometimes the taskrun don't want to use hostnetwork, and set it 'false' in task template explicitly. If the value of global template is 'true', it will be set to 'true'.
Changes
Right, now when you specify a default pod template and also specify
pod template at pipelinerun/taskrun level. The default pod template is
getting overridden by the one at the pipelinerun/taskrun level.
This changes this behavior by only overriding the
PodTemplate
fieldthat are defined by the
TaskRun
and not the wholePodTemplate
. This means it is possible to define aNodeSelector
widely (through the default) without it being overriden when the user
sets a
SecurityContext
on hisTaskRun
.This is also done on the
PipelineRun
PodTemplate.Closes #3569
/cc @sbwsg @afrittoli @pritidesai @skaegi @bobcatfish
This probably need a bit more unit tests, but I think it's worth opening to discuss if the approach is correct (for each fields)
Signed-off-by: Vincent Demeester vdemeest@redhat.com
/kind feature
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
functionality, content, code)
Release Notes