Add functions to merge step/sidecar overrides #4617
Conversation
tekton-robot
added
release-note-none
tekton-robot
added
the
size/XL
|
The following is the coverage report on the affected files.
|
|
/test pull-tekton-pipeline-integration-tests |
|
/assign |
| @@ -31,61 +31,111 @@ func MergeStepsWithStepTemplate(template *v1.Container, steps []Step) ([]Step, e | |||
| return steps, nil | |||
| } | |||
|
|
|||
| // We need JSON bytes to generate a patch to merge the step containers | |||
| // onto the template container, so marshal the template. | |||
| templateAsJSON, err := json.Marshal(template) | |||
There was a problem hiding this comment.
Probably a bit of an optimization but can we pass the marshall object in mergeObject instead of the template string ? Before this merge we would marshall the template once and the use it in the loop. With this change, we will marshall the template for each steps, even though it never changes.
There was a problem hiding this comment.
Done! Note that the template doesn't change for StepTemplates, but it does change for StepOverrides, so I've provided two separate functions
lbernick
force-pushed
the
resource-overrides-merge
branch
from
9405cbb to
b6bfb9c
Compare
|
The following is the coverage report on the affected files.
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vdemeester 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 |
tekton-robot
added
the
approved
lbernick
force-pushed
the
resource-overrides-merge
branch
from
b6bfb9c to
8d51ab4
Compare
|
The following is the coverage report on the affected files.
|
lbernick
force-pushed
the
resource-overrides-merge
branch
from
8d51ab4 to
1fe1d09
Compare
|
The following is the coverage report on the affected files.
|
lbernick
force-pushed
the
resource-overrides-merge
branch
from
1fe1d09 to
2912e5b
Compare
|
The following is the coverage report on the affected files.
|
|
/test tekton-pipeline-unit-tests |
|
/test pull-tekton-pipeline-alpha-integration-tests |
This commit adds methods to merge a slice of Step overrides with a slice of Steps. This functionality is necessary for the full implementation of Step and Sidecar overrides. It re-uses the merge process that is used to combine Steps and Step templates; however, the Step is treated as the "template" so that the Step override takes precedence. We do not attempt to validate the resource requirements of the resulting pod; it's possible for the resulting pod to have resource requests > limits, which k8s will reject. This is the same way we currently handle Step template.
lbernick
force-pushed
the
resource-overrides-merge
branch
from
2912e5b to
a424adb
Compare
|
Awesome, cheers @lbernick ! /lgtm |
|
The following is the coverage report on the affected files.
|
|
/test pull-tekton-pipeline-alpha-integration-tests |
1 similar comment
|
/test pull-tekton-pipeline-alpha-integration-tests |
Changes
This commit adds methods to merge a slice of Step overrides with a slice of Steps.
This functionality is necessary for the full implementation of Step and Sidecar overrides.
It re-uses the merge process that is used to combine Steps and Step templates;
however, the Step is treated as the "template" so that the Step override takes precedence.
We do not attempt to validate the resource requirements of the resulting pod;
it's possible for the resulting pod to have resource requests > limits, which k8s will reject.
This is the same way we currently handle Step template.
Part 2 of #4326.
This is part of #4598, split out into its own PR.
/kind feature
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
functionality, content, code)
Release Notes