Reduce the number of imageDigestExporter step to one per Task #1126
Conversation
googlebot
added
the
cla: yes
tekton-robot
added
approved
|
/hold |
tekton-robot
added
the
do-not-merge/hold
Currently, if a `Task` uses one or more image output we add a `imageDigestExporter` for each step defined in the `Task`. This is not required as only one step/container can do the work and handle multiple images. This change reduce to one the number of `imageDigestExporter` to one step, after the last user-defined step. Signed-off-by: Vincent Demeester <vdemeest@redhat.com>
vdemeester
force-pushed
the
1104-imagedigest-only-once-per-taskrun
branch
from
b7b5cba
to
22acced
Compare
|
The following is the coverage report on pkg/.
|
|
I'm trying to remember the details of the reason we had an image after each step - I even paired with @nader-ziada on this and I can't quite remember 🤣 I'm really kicking myself that we didn't make it clear in comments :S Part of the reason was that it seemed like we couldn't guarantee that we knew which step was exporting the image - maybe to handle a case where we export 2 images, each in a different step?? But it sounds like we're handling that already - all I can come up with is that maybe it was to handle a case where the same image is getting exported multiple times but I'm not sure now why we'd want to support that 🤷♀ Might as well go ahead with this for now unless @nader-ziada is around and can remember why we did it? 😅 |
|
Agree with @bobcatfish it had to do with building same image in multiple steps, but makes sense to me to have just one step in the end. |
There was a problem hiding this comment.
This looks good to me, but I feel we need to document what type of restrictions we have in terms of using the image pipeline resources and build the output.
I think it makes sense to have a Task producing one image - multi-stage docker files can be used where needed, so in general only one output of type image should be available in a Task.
It's fine to do that as a follow-up.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: afrittoli, dibyom, 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 |
Well, it's possible for a task to build and push multiple images 👼 |
| augmentedSteps = append(augmentedSteps, imageDigestExporterContainer(s.Name, imagesJSON)) | ||
| } | ||
| augmentedSteps = append(augmentedSteps, taskSpec.Steps...) | ||
| augmentedSteps = append(augmentedSteps, imageDigestExporterContainer(imagesJSON)) |
There was a problem hiding this comment.
at this point we probably don't even need augmentedSteps ? could use taskSpec.Steps directly
|
/meow space |
|
In response to this:
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. |
|
/hold cancel |
tekton-robot
removed
the
do-not-merge/hold
Changes
Currently, if a
Taskuses one or more image output we add aimageDigestExporterfor each step defined in theTask. This is notrequired as only one step/container can do the work and handle
multiple images.
This change reduce to one the number of
imageDigestExporterto onestep, after the last user-defined step.
Closes #1104
Signed-off-by: Vincent Demeester vdemeest@redhat.com
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide for more details.
Double check this list of stuff that's easy to miss:
cmddir, please updatethe release Task and TaskRun to build and release this image
Release Notes