-
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
Extract substitution code out of v1alpha1 β #1611
Conversation
The following is the coverage report on pkg/.
|
/test tekton-pipeline-unit-tests |
/lgtm |
/lgtm |
[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 |
This part doesn't really have anything to do with the API so, moving it out of the v1alpha1 api package. Signed-off-by: Vincent Demeester <vdemeest@redhat.com>
ed33052
to
7a7c34b
Compare
arf, another rebase π€¦ββοΈ |
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.
it just keeps getting better and better :D
/lgtm
@@ -15,13 +15,13 @@ See the License for the specific language governing permissions and | |||
limitations under the License. | |||
*/ | |||
|
|||
package v1alpha1_test | |||
package substitution_test |
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.
total nitpick but i think we might be able to get away with just substitution
now? afaik v1alpha_test
had to be used b/c we were having some kinda like conflicts or circular dependency problems or something (might be thinking of a different issue tho - i know this happens with the builder libs a lot! - but probably wont once you're done!!)
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.
I think v1alpha_test
might have been for that reason indeed. I tend to like using _test
package as it force the test to only use what the user can use and how he will use it π
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.
ah kk! makes sense to me. ill have to noodle on it a bit and maybe ill switch to the same thing!
The following is the coverage report on pkg/.
|
Changes
This part doesn't really have anything to do with the API so, moving
it out of the v1alpha1 api package.
This is part of the "cleaning v1alpha1 and create v1alpha2" work π
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:
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.