Break Pipelines' dependency on Build #648

vdemeester · 2019-03-21T14:39:19Z

Changes

This removes the dependencies on knative/build. Only pkg/reconcilier/v1alpha1/taskrun/… packages were the last packages, and now there are not depending on build anymore.

With this "Phase 2" is effectively complete !

~~Marking as WIP as I feel there is still room for improvements on the refactoring I did — and the commits in there are not final, I'll rebase and squash before we can merge 😉.~~

Closes #252

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)
~~[ ] Includes docs (if user facing)~~
Commit messages follow commit message best practices

See the contribution guide
for more details.

Release Notes

pipeline doesn't depend on knative/build anymore 💃

tekton-robot · 2019-03-21T14:39:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vdemeester

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:

~~OWNERS~~ [vdemeester]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

bobcatfish

Looking great!!

My main feedback is that there are a few function + test names that I think we could update to no longer include Build (even tho its super tedious 😩 ) (also I dont think i pointed them all out but I caught some of them!) but mostly looks great!!! 😻

pkg/reconciler/v1alpha1/taskrun/taskrun.go

pkg/reconciler/v1alpha1/taskrun/resources/pod.go

bobcatfish · 2019-03-21T18:36:50Z

pkg/reconciler/v1alpha1/taskrun/taskrun.go

 	if err != nil {
 		return nil, fmt.Errorf("failed to add entrypoint to steps of TaskRun %s: %v", tr.Name, err)
 	}
 	// Add the step which will copy the entrypoint into the volume
 	// we are going to be using, so that all of the steps will have
 	// access to it.
-	entrypoint.AddCopyStep(bs)
-	b := &buildv1alpha1.Build{


oh weird, we were instantiating Build twice ! 🤔

pkg/reconciler/v1alpha1/taskrun/taskrun_test.go

bobcatfish · 2019-03-21T18:43:23Z

pkg/reconciler/v1alpha1/taskrun/resources/pod_test.go

-					Type:     v1alpha1.GCSManifest,
-					Location: "gs://foo/bar",
-				},
-				TargetPath: "path/foo",


do we still need TargetPath covered in this test, using resources (https://github.com/tektoncd/pipeline/blob/3e8b177fbeb39126a9c9de121008db571ef45bfa/docs/tasks.md#controlling-where-resources-are-mounted) ? (im not sure myself!)

I think it's already covered with resources. This code was testing something we never set as is anymore I think 👼

pkg/reconciler/v1alpha1/taskrun/resources/pod.go

pkg/reconciler/v1alpha1/taskrun/resources/input_resource_test.go

imjasonh · 2019-03-25T17:22:21Z

Gopkg.toml

@@ -67,10 +67,6 @@ required = [
  name = "go.uber.org/zap"
  revision = "67bc79d13d155c02fd008f721863ff8cc5f30659"

-[[constraint]]
-  name = "github.com/knative/build"


vdemeester · 2019-03-25T17:42:56Z

/test pull-tekton-pipeline-integration-tests

bobcatfish

Just one super minor thing (some commented out code 😭 lol wanted to lgtm ... ) otherwise looks great!!

/meow space

pkg/reconciler/v1alpha1/taskrun/resources/pod.go

tekton-robot · 2019-03-25T23:30:50Z

@bobcatfish:

In response to this:

Just one super minor thing (some commented out code 😭 lol wanted to lgtm ... ) otherwise looks great!!

/meow space

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.

This removes the latest piece of dependencies on `knative/build` that were in `pkg/reconcilier/v1alpha1/taskrun/…` packages. `TaskRun`/`TaskSpec` are no more translated to `Build` types, that are used to create `Pod` later on. We now create `Pod` directly from `TaskRun`/`TaskSpec` definitions. This also remove test builder code related to those types. Signed-off-by: Vincent Demeester <vdemeest@redhat.com>

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

bobcatfish · 2019-03-26T17:37:57Z

/lgtm

vdemeester · 2019-03-26T20:30:38Z

/test pull-tekton-pipeline-integration-tests

tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 21, 2019

googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Mar 21, 2019

tekton-robot requested review from dlorenc and imjasonh March 21, 2019 14:39

tekton-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 21, 2019

vdemeester force-pushed the 252-break-build-dep-2 branch from a024fd2 to 4dfdbac Compare March 21, 2019 17:00

vdemeester changed the title ~~WIP: Break Pipelines' dependency on Build~~ Break Pipelines' dependency on Build Mar 21, 2019

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 21, 2019

bobcatfish reviewed Mar 21, 2019

View reviewed changes

vdemeester force-pushed the 252-break-build-dep-2 branch 3 times, most recently from e49c72b to 72dd6d2 Compare March 25, 2019 17:01

imjasonh reviewed Mar 25, 2019

View reviewed changes

bobcatfish reviewed Mar 25, 2019

View reviewed changes

pkg/reconciler/v1alpha1/taskrun/resources/pod.go Outdated Show resolved Hide resolved

vdemeester force-pushed the 252-break-build-dep-2 branch from 72dd6d2 to dedb5dc Compare March 26, 2019 07:57

vdemeester added 2 commits March 26, 2019 09:25

Update dependencies — effectively removing knative/build as a dependency

e8d1e55

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

vdemeester force-pushed the 252-break-build-dep-2 branch from dedb5dc to e8d1e55 Compare March 26, 2019 08:25

vdemeester added 2 commits March 26, 2019 10:43

Remove some unused constants ⛏

029f7b2

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

Remove build.knative.dev from clusterrole yaml

aeff9b5

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

tekton-robot assigned bobcatfish Mar 26, 2019

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 26, 2019

tekton-robot merged commit 2f7dbd9 into tektoncd:master Mar 26, 2019

vdemeester deleted the 252-break-build-dep-2 branch March 26, 2019 21:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Break Pipelines' dependency on Build #648

Break Pipelines' dependency on Build #648

vdemeester commented Mar 21, 2019 •

edited

Loading

tekton-robot commented Mar 21, 2019

bobcatfish left a comment •

edited

Loading

bobcatfish Mar 21, 2019

bobcatfish Mar 21, 2019

vdemeester Mar 22, 2019

bobcatfish Mar 25, 2019

imjasonh Mar 25, 2019

vdemeester commented Mar 25, 2019

bobcatfish left a comment

tekton-robot commented Mar 25, 2019

bobcatfish commented Mar 26, 2019

vdemeester commented Mar 26, 2019

Break Pipelines' dependency on Build #648

Break Pipelines' dependency on Build #648

Conversation

vdemeester commented Mar 21, 2019 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Mar 21, 2019

bobcatfish left a comment • edited Loading

Choose a reason for hiding this comment

bobcatfish Mar 21, 2019

Choose a reason for hiding this comment

bobcatfish Mar 21, 2019

Choose a reason for hiding this comment

vdemeester Mar 22, 2019

Choose a reason for hiding this comment

bobcatfish Mar 25, 2019

Choose a reason for hiding this comment

imjasonh Mar 25, 2019

Choose a reason for hiding this comment

vdemeester commented Mar 25, 2019

bobcatfish left a comment

Choose a reason for hiding this comment

tekton-robot commented Mar 25, 2019

bobcatfish commented Mar 26, 2019

vdemeester commented Mar 26, 2019

vdemeester commented Mar 21, 2019 •

edited

Loading

bobcatfish left a comment •

edited

Loading