Declare expected volumes explicitly 🗣️ #1481
Conversation
googlebot
added
the
cla: yes
tekton-robot
added
the
size/M
|
I am definitely missing some kinda linting in my editor 🤔 |
The test was dynamically adding the expected PVC volumes. This created two problems: 1. If there was a bug in the logic that was adding them dynamically, we we could end up covering a bug 2. This made it confusing to try to reason about what the actual expected pod structure was. In tektoncd#1417 I had to make a lot of changes to these tests and it was super confusing to look at the expected structure and sometimes see no volumes (or even volumes: nil) when you knew volumes were supposed to be there (I was scared there was a serious bug for a while) So now the expected state explicitly lists all the volumes it expects. This also revealed that one test case was using `pipelinerun-parent` as the name of the parent pipelinerun even though all the rest used just `pipelinerun` which was also confusing when I was making changes b/c I initially thought there was some special case where tekton itself would add "parent" but no.
bobcatfish
force-pushed
the
arbitrarily_different_string
branch
from
c1cbb4c
to
b0aef0d
Compare
|
/lgtm |
|
/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 |
tekton-robot
added
the
approved
|
@bobcatfish: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
|
oh dear; merge conflicts appear to also break the test runnings |
|
After looking into the merge conflict, it looks like in ffadda1#diff-db3818c61ae7be4751c7dfe8fc32f74e @dlorenc and I made this exact same change (with some questionable whitespace / newline use, but que sera sera XD So i'm gonna close this! |
Changes
The test was dynamically adding the expected PVC volumes. This created
two problems:
we could end up covering a bug
expected pod structure was. In Add "volume" PipelineResource 🔊 #1417 I had to make a lot of changes
to these tests and it was super confusing to look at the expected
structure and sometimes see no volumes (or even volumes: nil) when
you knew volumes were supposed to be there (I was scared there was a
serious bug for a while)
So now the expected state explicitly lists all the volumes it expects.
This also revealed that one test case was using
pipelinerun-parentasthe name of the parent pipelinerun even though all the rest used just
pipelinerunwhich was also confusing when I was making changes b/c Iinitially thought there was some special case where tekton itself would
add "parent" but no.
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 to build and release this image.
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.