Skip to content
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

tests: import pipeline internal builders 🏒 #1177

Merged
merged 1 commit into from
Sep 18, 2020

Conversation

vdemeester
Copy link
Member

Changes

Importing pipeline internal builders in cli so that we can safely take
time to migrate out of them with less pressure (and with less
dependency problems).

This doesn't prevent to fix #1145, it's just putting less pressure on getting it done before the next release 😉

Signed-off-by: Vincent Demeester vdemeest@redhat.com

/cc @piyush-garg @danielhelfand @pradeepitm12

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

  • Includes tests (if functionality changed/added)
  • Run the code checkers with make check
  • Regenerate the manpages, docs and go formatting with make generated
  • Commit messages follow commit message best practices

See the contribution guide
for more details.

/kind misc

@tekton-robot
Copy link
Contributor

@vdemeester: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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.

@tekton-robot tekton-robot added kind/misc Categorizes issue or PR as a miscellaneuous one. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 18, 2020
@vdemeester
Copy link
Member Author

/release-note-none

@tekton-robot tekton-robot added release-note-none Denotes a PR that doesnt merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Sep 18, 2020
@tekton-robot
Copy link
Contributor

The following is the coverage report on the affected files.
Say /test pull-tekton-cli-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
internal/builder/v1alpha1/condition.go Do not exist 100.0%
internal/builder/v1alpha1/param.go Do not exist 100.0%
internal/builder/v1alpha1/pipeline.go Do not exist 83.6%
internal/builder/v1alpha1/sidecar.go Do not exist 66.7%
internal/builder/v1alpha1/step.go Do not exist 19.0%
internal/builder/v1alpha1/task.go Do not exist 66.7%
internal/builder/v1beta1/container.go Do not exist 91.7%
internal/builder/v1beta1/owner_reference.go Do not exist 100.0%
internal/builder/v1beta1/param.go Do not exist 100.0%
internal/builder/v1beta1/pipeline.go Do not exist 81.2%
internal/builder/v1beta1/pod.go Do not exist 78.0%
internal/builder/v1beta1/resource.go Do not exist 88.9%
internal/builder/v1beta1/sidecar.go Do not exist 66.7%
internal/builder/v1beta1/step.go Do not exist 19.0%
internal/builder/v1beta1/task.go Do not exist 75.5%

@vdemeester vdemeester force-pushed the import-internal-builder branch 2 times, most recently from c5efa65 to 7ea3a39 Compare September 18, 2020 10:41
@tekton-robot
Copy link
Contributor

The following is the coverage report on the affected files.
Say /test pull-tekton-cli-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
internal/builder/v1alpha1/condition.go Do not exist 100.0%
internal/builder/v1alpha1/param.go Do not exist 100.0%
internal/builder/v1alpha1/pipeline.go Do not exist 83.6%
internal/builder/v1alpha1/sidecar.go Do not exist 66.7%
internal/builder/v1alpha1/step.go Do not exist 19.0%
internal/builder/v1alpha1/task.go Do not exist 66.7%
internal/builder/v1beta1/container.go Do not exist 91.7%
internal/builder/v1beta1/owner_reference.go Do not exist 100.0%
internal/builder/v1beta1/param.go Do not exist 100.0%
internal/builder/v1beta1/pipeline.go Do not exist 81.2%
internal/builder/v1beta1/pod.go Do not exist 78.0%
internal/builder/v1beta1/resource.go Do not exist 88.9%
internal/builder/v1beta1/sidecar.go Do not exist 66.7%
internal/builder/v1beta1/step.go Do not exist 19.0%
internal/builder/v1beta1/task.go Do not exist 75.5%

@tekton-robot
Copy link
Contributor

The following is the coverage report on the affected files.
Say /test pull-tekton-cli-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
internal/builder/v1alpha1/condition.go Do not exist 100.0%
internal/builder/v1alpha1/param.go Do not exist 100.0%
internal/builder/v1alpha1/pipeline.go Do not exist 83.6%
internal/builder/v1alpha1/sidecar.go Do not exist 66.7%
internal/builder/v1alpha1/step.go Do not exist 19.0%
internal/builder/v1alpha1/task.go Do not exist 66.7%
internal/builder/v1beta1/container.go Do not exist 91.7%
internal/builder/v1beta1/owner_reference.go Do not exist 100.0%
internal/builder/v1beta1/param.go Do not exist 100.0%
internal/builder/v1beta1/pipeline.go Do not exist 81.2%
internal/builder/v1beta1/pod.go Do not exist 78.0%
internal/builder/v1beta1/resource.go Do not exist 88.9%
internal/builder/v1beta1/sidecar.go Do not exist 66.7%
internal/builder/v1beta1/step.go Do not exist 19.0%
internal/builder/v1beta1/task.go Do not exist 75.5%

@tekton-robot
Copy link
Contributor

The following is the coverage report on the affected files.
Say /test pull-tekton-cli-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
internal/builder/v1alpha1/condition.go Do not exist 100.0%
internal/builder/v1alpha1/param.go Do not exist 100.0%
internal/builder/v1alpha1/pipeline.go Do not exist 83.6%
internal/builder/v1alpha1/sidecar.go Do not exist 66.7%
internal/builder/v1alpha1/step.go Do not exist 19.0%
internal/builder/v1alpha1/task.go Do not exist 66.7%
internal/builder/v1beta1/container.go Do not exist 91.7%
internal/builder/v1beta1/owner_reference.go Do not exist 100.0%
internal/builder/v1beta1/param.go Do not exist 100.0%
internal/builder/v1beta1/pipeline.go Do not exist 81.2%
internal/builder/v1beta1/pod.go Do not exist 78.0%
internal/builder/v1beta1/resource.go Do not exist 88.9%
internal/builder/v1beta1/sidecar.go Do not exist 66.7%
internal/builder/v1beta1/step.go Do not exist 19.0%
internal/builder/v1beta1/task.go Do not exist 75.5%

@tekton-robot
Copy link
Contributor

The following is the coverage report on the affected files.
Say /test pull-tekton-cli-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
internal/builder/v1alpha1/condition.go Do not exist 100.0%
internal/builder/v1alpha1/param.go Do not exist 100.0%
internal/builder/v1alpha1/pipeline.go Do not exist 83.6%
internal/builder/v1alpha1/sidecar.go Do not exist 66.7%
internal/builder/v1alpha1/step.go Do not exist 19.0%
internal/builder/v1alpha1/task.go Do not exist 66.7%
internal/builder/v1beta1/container.go Do not exist 91.7%
internal/builder/v1beta1/owner_reference.go Do not exist 100.0%
internal/builder/v1beta1/param.go Do not exist 100.0%
internal/builder/v1beta1/pipeline.go Do not exist 81.2%
internal/builder/v1beta1/pod.go Do not exist 78.0%
internal/builder/v1beta1/resource.go Do not exist 88.9%
internal/builder/v1beta1/sidecar.go Do not exist 66.7%
internal/builder/v1beta1/step.go Do not exist 19.0%
internal/builder/v1beta1/task.go Do not exist 75.5%

@vdemeester
Copy link
Member Author

/test pull-tekton-cli-integration-tests
/test pull-tekton-cli-build-tests

@piyush-garg
Copy link
Contributor

piyush-garg commented Sep 18, 2020

/lgtm

One question, are these builders from 0.16?

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 18, 2020
@vdemeester
Copy link
Member Author

/lgtm

One question, are these builders from 1.16?

0.15.2 (because they were removed before 0.16.0)

@piyush-garg
Copy link
Contributor

piyush-garg commented Sep 18, 2020

/lgtm
One question, are these builders from 1.16?

0.15.2 (because they were removed before 0.16.0)

But the internal is there in 0.16 right? They are removed from test/builder pkg

Copy link
Contributor

@pradeepitm12 pradeepitm12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@vdemeester
Copy link
Member Author

/lgtm
One question, are these builders from 1.16?

0.15.2 (because they were removed before 0.16.0)

But the internal is there in 0.16 right? They are removed from test/builder pkg

yeah it's there lol, that's a good point. I can bump it to 0.16.0 if needed in a follow-up 😝

Copy link
Member

@danielhelfand danielhelfand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

We should also announce in release notes for v0.13.0 that users should not develop around these test builders as they will not be supported long term. But think it makes sense to add these for the time being to more gracefully move away from them.

@tekton-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danielhelfand

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 18, 2020
@piyush-garg
Copy link
Contributor

/lgtm

We should also announce in release notes for v0.13.0 that users should not develop around these test builders as they will not be supported long term. But think it makes sense to add these for the time being to more gracefully move away from them.

I don't think people are using anything from CLI to use builders.

@vdemeester
Copy link
Member Author

ah, that needs a rebase 😅

@danielhelfand
Copy link
Member

Yeah, from #1168.

Importing pipeline internal builders in cli so that we can safely take
time to migrate out of them with less pressure (and with less
dependency problems).

Signed-off-by: Vincent Demeester <vdemeest@redhat.com>
@tekton-robot tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 18, 2020
@vdemeester
Copy link
Member Author

Rebased 😉 👼 🙃

@tekton-robot
Copy link
Contributor

The following is the coverage report on the affected files.
Say /test pull-tekton-cli-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
internal/builder/v1alpha1/condition.go Do not exist 100.0%
internal/builder/v1alpha1/param.go Do not exist 100.0%
internal/builder/v1alpha1/pipeline.go Do not exist 83.6%
internal/builder/v1alpha1/sidecar.go Do not exist 66.7%
internal/builder/v1alpha1/step.go Do not exist 19.0%
internal/builder/v1alpha1/task.go Do not exist 66.7%
internal/builder/v1beta1/container.go Do not exist 91.7%
internal/builder/v1beta1/owner_reference.go Do not exist 100.0%
internal/builder/v1beta1/param.go Do not exist 100.0%
internal/builder/v1beta1/pipeline.go Do not exist 81.2%
internal/builder/v1beta1/pod.go Do not exist 78.0%
internal/builder/v1beta1/resource.go Do not exist 88.9%
internal/builder/v1beta1/sidecar.go Do not exist 66.7%
internal/builder/v1beta1/step.go Do not exist 19.0%
internal/builder/v1beta1/task.go Do not exist 75.5%

@piyush-garg
Copy link
Contributor

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 18, 2020
@piyush-garg
Copy link
Contributor

/retest

@tekton-robot tekton-robot merged commit 3bdd4ae into tektoncd:master Sep 18, 2020
@vdemeester vdemeester deleted the import-internal-builder branch September 18, 2020 16:32
danielhelfand added a commit that referenced this pull request Sep 30, 2020
#1167 | [Daniel Helfand] update condition tests from test builders to structs | 2020/09/16-11:10
#1154 | [vinamra28] Enable auto-select support in ClusterTaskDescribe if only one is present | 2020/09/16-17:00
#1154 | [vinamra28] Enable auto-select support in TaskRunDescribe if only one is present | 2020/09/16-17:00
#1173 | [Daniel Helfand] update README versions for v0.12.1 | 2020/09/16-17:21
#1176 | [Bart] Artwork/Logo added to the top of the README. | 2020/09/16-18:14
#1172 | [Chmouel Boudjnah] Fix rpm build for release | 2020/09/17-14:34
#1168 | [Daniel Helfand] update clustertask tests from test builders to structs | 2020/09/18-13:26
#1177 | [Vincent Demeester] tests: import pipeline internal builders 🏒 | 2020/09/18-17:31
#1179 | [vinamra28] Use --prefix-name option for tkn clustertask start | 2020/09/18-23:07
#1182 | [Daniel Helfand] update pipelineresource tests from test builders to structs | 2020/09/21-14:29
null | [Vincent Demeester] Remove release-note block indentation in PR template 🌮 | 2020/09/22-11:57
null | [savitaashture] Modify tkn version to accept ldflag and namespace flag | 2020/09/24-10:14
null | [savitaashture] Fix deployment fetch issue for multiple namespaces | 2020/09/25-11:54
null | [PuneetPunamiya] This patch fixes: | 2020/09/28-19:27
null | [Divyansh42] Modify tkn version output to hide Triggers and Dashboard version if they are not installed and added required unit tests. | 2020/09/29-04:57
null | [Piyush Garg] Bump pipeline and triggers dep | 2020/09/29-13:55
null | [Divyansh42] Enable auto-select support in pipelineDescribe if only one pipeline is present | 2020/09/29-17:29
null | [vinamra28] Add --use-taskrun for ClusterTask start | 2020/09/30-11:08
null | [Divyansh42] Enable auto select support in PipelineRunDescribe if only one PipelineRun is present | 2020/09/30-13:13

Signed-off-by: Daniel Helfand <helfand.4@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/misc Categorizes issue or PR as a miscellaneuous one. lgtm Indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesnt merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move from pipeline builders to structs
5 participants