-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Migrate some test builder references to structs #3124
Conversation
The following is the coverage report on the affected files.
|
Also delete test/builder/ since it appears to be unused
The following is the coverage report on the affected files.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall seems good to me.
Also delete
test/builder/
since it appears to be unused.
Yes, it was kept here to not break downstream consumer of this package. I think it should be ok to remove it now (cc @piyush-garg @pradeepitm12 @dibyom for cli and triggers)
Thanks for flagging @vdemeester I think triggers still might be using some pipeline builders but I'll try to get rid of them. In any case, I think this PR can be merged -- won't break us until we update pipeline dependency |
ping @piyush-garg @pradeepitm12 any concerrns with this change? |
This may require some changes in the tests of cli, but that can be handled. |
@imjasonh This may break CLI tests whenever the next bump happens. We are not using the internal pkg, but are dependent on test/builder for CLI tests. I have created an issue in CLI to move to structs. tektoncd/cli#1145 |
So it sounds like people are generally okay with removing |
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dibyom 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 |
Also delete
test/builder/
since it appears to be unused.Before:
After:
/kind cleanup
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:
cmd
dir, 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.
Release Notes
/assign @vdemeester