TaskRunStatus includes sidecar status #1515
Conversation
googlebot
added
the
cla: yes
tekton-robot
added
size/S
|
Hi @fraenkel. Thanks for your PR. I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
|
/ok-to-test |
tekton-robot
added
ok-to-test
pkg/status/taskrunpod.go
Outdated
| @@ -55,8 +56,16 @@ func UpdateStatusFromPod(taskRun *v1alpha1.TaskRun, pod *corev1.Pod, resourceLis | |||
| ContainerName: s.Name, | |||
| ImageID: s.ImageID, | |||
| }) | |||
| } else { | |||
There was a problem hiding this comment.
Non-blocking idea for a future change in this area: We should prefix sidecar containers with something like sidecar- like we do for step containers, to prevent possible collisions, and so we can definitively identify them here (IsSidecarStep). In the future we might have some third type of container we might want to identify separately.
(Not for this PR, but if you felt like doing it that'd be fine. Otherwise I can file an issue to track it for later)
There was a problem hiding this comment.
I will do it in a separate commit. If its on the larger side I will just split it off into a separate PR.
pkg/status/taskrunpod.go
Outdated
| } | ||
| } | ||
| if len(sidecars) > 0 { |
There was a problem hiding this comment.
You could just get rid of the sidecars slice and on line 60 do taskRun.Status.Sidecars = append(taskRun.Status.Sidecars, v1alpha1.SidecarState{...}) like we do for step containers.
There was a problem hiding this comment.
I did that first and didn't want to update all the tests, but I switched to be consistent.
|
The build failures indicate you need to run |
fraenkel
force-pushed
the
sidecar_status
branch
from
979918c
to
15387de
Compare
tekton-robot
added
size/M
Record the sidecar name and image id for posterity. Fixes tektoncd#1511
fraenkel
force-pushed
the
sidecar_status
branch
from
15387de
to
bb7d0b7
Compare
|
The following is the coverage report on pkg/.
|
Prefix all sidecar container names with 'sidecar-' Counting, stopping and status collection of sidecar status will all look at the container name to determine if the operation applies.
fraenkel
force-pushed
the
sidecar_status
branch
from
21b7535
to
55debdb
Compare
|
The following is the coverage report on pkg/.
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ImJasonH 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
Changes
Record the sidecar name and image id for posterity.
Fixes #1511
I didn't include the container name because it is the same as the name.
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.
Reviewer Notes
If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.
Release Notes