Versions for builders and internal package 📦 #2537
Conversation
tekton-robot
added
the
size/XXL
vdemeester
force-pushed
the
internal-builder
branch
from
4655e2d to
db67e09
Compare
|
The following is the coverage report on the affected files.
|
|
@vdemeester i think you tried to explain this to me in chat but I don't quite understand why we need 2 versions of the builders? esp. if we're going to be able to rely on having only 1 type to interact with in the tekton controller, which is my understanding of the finished state of updating the storage types.
Or is the idea that the next step would be to delete the v1alpha1 builders? |
|
I would like to take this opportunity to commit to a goal of phasing out test builders entirely, after these changes are in place. Phasing out test builders shouldn't block "real" work like updating the storage version, but since this will move them to If this seems reasonable I can write a real issue to track it, with justifications and timelines, and assign it to myself. If not, I'd like to convince you that it is reasonable and still do it. 😄 |
I think test code will be more transparent without the builders. It may be non-obvious in some cases how to build an object, but a good reference example should be enough to cover that. |
@bobcatfish There is multiple answer to the "2 versions". There is 2 versions of the APIs, if we do builder for the api it make sense to have 2 version of the builder — I mean, why not, especially if we use them in tests (we are testing v1alpha1 and v1beta1 in e2e for example). But, yes the idea is the "next" step (after #2410) will be to remove the
@imjasonh Yes 👍 We may need alternative for verbose cases (a The main goal here is:
@afrittoli So the goal is to have test readable and with the least "noise". That said, builders didn't really achieve what I had in mind initially… 😓 |
vdemeester
force-pushed
the
internal-builder
branch
from
db67e09 to
b86858c
Compare
|
The linting should be fixed 🤞 |
|
The following is the coverage report on the affected files.
|
vdemeester
force-pushed
the
internal-builder
branch
from
b86858c to
a658a19
Compare
|
The following is the coverage report on the affected files.
|
This adds versions for the builder: v1alpha1 and v1beta1. This also migrate the builder to `internal/builder` while keeping an alias on `test/builder` for user of this package (marking it as deprecated). Signed-off-by: Vincent Demeester <[email protected]>
vdemeester
force-pushed
the
internal-builder
branch
from
a658a19 to
339713d
Compare
|
The following is the coverage report on the affected files.
|
|
Hard to review the full thing but from what I understand it is just a migration between alpha to beta until we get rid of the so that sounds good to me, /lgtm |
| cleanup) of the tests should be as small as possible to avoid the noise. Those | ||
| builders exists to help with that. | ||
|
|
||
| There is currently two versionned builder supported: |
There was a problem hiding this comment.
NIT: There are currently two versioned builders supported:
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: afrittoli 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
|
/kind misc |
|
/test pull-tekton-pipeline-integration-tests |
Changes
This adds versions for the builder: v1alpha1 and v1beta1.
This also migrate the builder to
internal/builderwhile keeping analias on
test/builderfor user of this package (marking it asdeprecated).
Those are in the
internalpackage so only this project can refer them (making it easier to remove later on as nobody from the outside will depend on them).This is in preparation of #2410 as… well… builders are gonna be needed for v1beta1 (or… it will take ages to do 😹 ).
This feels big but there is not much net new code:
test/builder->internal/builder/v1alpha1)internal/builder/v1beta1) based oninternal/builder/v1alpha1but with the v1beta1 APISigned-off-by: Vincent Demeester [email protected]
/cc @afrittoli @dibyom @bobcatfish @imjasonh
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.