Migrate PipelineResource in their own package… 📦 #1773

vdemeester · 2019-12-19T15:36:41Z

Changes

Extracts PipelineResource in their own package while still being exposed in tekton.dev. This is require to support embedded PipelineResource in v1alpha2 with PipelineResource still being alpha (aka v1alpha1).

This adds a pkg/apis/resource/v1alpha1 package which holds PipelineResource struct and related.
This adds a pkg/client/resource package (in addition to the pkg/client package) for the PipelineResource generated clients, lister, … code.

… and API group (resource.tekton.dev). This is mainly done to
separate the PipelineResource from the rest of the API and remove
the dependency cycle that we get by keeping PipelineResource in
alpha.

This doesn't change yet the current webhook and controller, and does
not expose PipelineResource.resource.tekton.dev (current exposed are
PipelineResource.tekton.dev).

We can expose this and support this once we expose v1alpha2 (or
v1beta1). We will need to auto-convert PipelineResource.tekton.dev
to PipelineResource.resource.tekton.dev, the easiest way would be to
have a controller handling that.

This comes from this comment #1725 (comment) 👼 .

This is an experiment, seeing if it make sense. Without this, it is very difficult to support embedded PipelineResource in v1alpha2 (or I am missing something really obvious 😝 )
/hold

/cc @bobcatfish

Signed-off-by: Vincent Demeester [email protected]

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.

Double check this list of stuff that's easy to miss:

If you are adding a new binary/image to the cmd dir, please update
the 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.

tekton-robot · 2019-12-19T15:39:53Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage
pkg/apis/resource/v1alpha1/pipeline_resource_defaults.go	Do not exist	0.0%
pkg/apis/resource/v1alpha1/pipelineresource_validation.go	Do not exist	86.8%
pkg/apis/resource/v1alpha1/register.go	Do not exist	0.0%

vdemeester · 2019-12-19T16:37:47Z

/test pull-tekton-pipeline-integration-tests

bobcatfish

Thanks again for taking on all this finicky work 🙏

This doesn't change yet the current webhook and controller, and does
not expose PipelineResource.resource.tekton.dev (current exposed are
PipelineResource.tekton.dev).

I think it makes a lot of sense to move the code, but I'm a bit confused by why we want to add a new API group - looking at our other types, and even looking at triggers types such as trigger bindings they are still in the tekton.dev group (or maybe this is a mistake and the group should be different?), so I don't quite understand why we want pipelineresources to be an exceptional case 🤔

bobcatfish · 2019-12-20T16:14:28Z

cmd/webhook/main.go

 		v1alpha1.SchemeGroupVersion.WithKind("Task"):             &v1alpha1.Task{},
 		v1alpha1.SchemeGroupVersion.WithKind("ClusterTask"):      &v1alpha1.ClusterTask{},
 		v1alpha1.SchemeGroupVersion.WithKind("TaskRun"):          &v1alpha1.TaskRun{},
 		v1alpha1.SchemeGroupVersion.WithKind("PipelineRun"):      &v1alpha1.PipelineRun{},
 		v1alpha1.SchemeGroupVersion.WithKind("Condition"):        &v1alpha1.Condition{},
+		v1alpha1.SchemeGroupVersion.WithKind("PipelineResource"): &v1alpha1.PipelineResource{},
+		// resourcev1alpha1.SchemeGroupVersion.WithKind("PipelineResource"): &resourcev1alpha1.PipelineResource{},


Maybe accidentally included? Or if this is the line that exposes the API and we want to uncomment it later, maybe a comment above with an issue number explaining?

bobcatfish · 2019-12-20T16:18:16Z

pkg/apis/resource/v1alpha1/pipelineresource_validation_test.go

+		})
+	}
+
+}


It seems like some of these files are duplicated vs. moved? e.g. would https://github.com/tektoncd/pipeline/blob/master/pkg/apis/pipeline/v1alpha1/pipelineresource_validation_test.go still exist after this PR?

vdemeester · 2019-12-20T17:50:34Z

I think it makes a lot of sense to move the code, but I'm a bit confused by why we want to add a new API group - looking at our other types, and even looking at triggers types such as trigger bindings they are still in the tekton.dev group (or maybe this is a mistake and the group should be different?), so I don't quite understand why we want pipelineresources to be an exceptional case thinking

Right, on triggers, I would really like having them in a different group (make it easier to detect the precense of trigger without having to know which types it exposes — right now.. | grep tekton.dev returns pipeline and triggers 😛).

The main problem I've got, so far, is that I did not manage to make the codegenerator(s) work with two distinct package serving the exact same api group (the clients are from one package only making the thing useless). I am still trying to figure out how to do that, as it would definitely simplify that pull request.

That said, I think we may discuss apiGroup in pipeline and in tekton in general 👼.

bobcatfish · 2019-12-20T19:45:41Z

Right, on triggers, I would really like having them in a different group (make it easier to detect the precense of trigger without having to know which types it exposes — right now.. | grep tekton.dev returns pipeline and triggers stuck_out_tongue).

Ah okay, so in that case maybe we should have pipeline.tekton.dev and triggers.tekton.dev - i.e. mirror our repo layout?

I'm still not sure it would make sense to put resources in it's own group 🤔 on the other hand that might fit well with some of the plans @sbwsg and I have been scheming 😈

The main problem I've got, so far, is that I did not manage to make the codegenerator(s) work with two distinct package serving the exact same api group (the clients are from one package only making the thing useless). I am still trying to figure out how to do that, as it would definitely simplify that pull request.

I think it'd be better if we could get this to work, esp. if the main problem is that the tool doesn't work. If you want to write up an issue and get any of the rest of us to take a stab at it just say the word!! happy to help :D

vdemeester · 2019-12-23T11:26:55Z

@bobcatfish should be fixed now, no more different apiGroup.

tekton-robot · 2019-12-23T11:29:21Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage
pkg/apis/resource/v1alpha1/pipeline_resource_defaults.go	Do not exist	0.0%
pkg/apis/resource/v1alpha1/pipelineresource_validation.go	Do not exist	86.8%
pkg/apis/resource/v1alpha1/register.go	Do not exist	0.0%

tekton-robot · 2019-12-23T11:30:30Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage
pkg/apis/resource/v1alpha1/pipeline_resource_defaults.go	Do not exist	0.0%
pkg/apis/resource/v1alpha1/pipelineresource_validation.go	Do not exist	86.8%
pkg/apis/resource/v1alpha1/register.go	Do not exist	0.0%

tekton-robot · 2019-12-24T08:47:33Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage
pkg/apis/resource/v1alpha1/pipeline_resource_defaults.go	Do not exist	0.0%
pkg/apis/resource/v1alpha1/pipelineresource_validation.go	Do not exist	86.8%
pkg/apis/resource/v1alpha1/register.go	Do not exist	0.0%

tekton-robot · 2019-12-24T09:02:20Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage
pkg/apis/resource/v1alpha1/pipeline_resource_defaults.go	Do not exist	0.0%
pkg/apis/resource/v1alpha1/pipelineresource_validation.go	Do not exist	86.8%
pkg/apis/resource/v1alpha1/register.go	Do not exist	0.0%

tekton-robot · 2019-12-24T09:18:19Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage
pkg/apis/resource/v1alpha1/pipeline_resource_defaults.go	Do not exist	0.0%
pkg/apis/resource/v1alpha1/pipelineresource_validation.go	Do not exist	86.8%
pkg/apis/resource/v1alpha1/register.go	Do not exist	0.0%

tekton-robot · 2019-12-24T09:27:01Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage
pkg/apis/resource/v1alpha1/pipeline_resource_defaults.go	Do not exist	0.0%
pkg/apis/resource/v1alpha1/pipelineresource_validation.go	Do not exist	86.8%
pkg/apis/resource/v1alpha1/register.go	Do not exist	0.0%

tekton-robot · 2019-12-24T12:01:20Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage
pkg/apis/resource/v1alpha1/pipeline_resource_defaults.go	Do not exist	0.0%
pkg/apis/resource/v1alpha1/pipelineresource_validation.go	Do not exist	86.8%
pkg/apis/resource/v1alpha1/register.go	Do not exist	0.0%

tekton-robot · 2020-01-09T13:24:52Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage
pkg/apis/resource/v1alpha1/pipeline_resource_defaults.go	Do not exist	0.0%
pkg/apis/resource/v1alpha1/pipelineresource_validation.go	Do not exist	86.8%
pkg/apis/resource/v1alpha1/register.go	Do not exist	0.0%

vdemeester · 2020-01-09T13:31:45Z

@bobcatfish @sbwsg this should be ready for review 👼

vdemeester · 2020-01-09T13:41:43Z

/hold cancel

This is mainly done to separate the `PipelineResource` from the rest of the API *and* remove the dependency cycle that we get by keeping `PipelineResource` in alpha. This update the code generation to generate a different set of clients, informers, clientsets, … for `resource`, although it is exposed in the same apiGroup. Signed-off-by: Vincent Demeester <[email protected]>

tekton-robot · 2020-01-09T13:52:23Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage
pkg/apis/resource/v1alpha1/pipeline_resource_defaults.go	Do not exist	0.0%
pkg/apis/resource/v1alpha1/pipelineresource_validation.go	Do not exist	86.8%
pkg/apis/resource/v1alpha1/register.go	Do not exist	0.0%

bobcatfish · 2020-01-09T17:55:31Z

hack/update-codegen.sh

 bash ${REPO_ROOT_DIR}/hack/generate-knative.sh "injection" \
  github.com/tektoncd/pipeline/pkg/client github.com/tektoncd/pipeline/pkg/apis \
  "pipeline:v1alpha1,v1alpha2" \
  --go-header-file ${REPO_ROOT_DIR}/hack/boilerplate/boilerplate.go.txt
+GOFLAGS="${OLDGOFLAGS}"


is it possible to add some comments around the enw lines in this script explaining what they do? and/or explain in the commit message? iirc from the previous review, it was not obvious what you had to do to make this work without a different apigroup, might be useful to record how this ended up working

ah this comes from #1783.. forgot to mention that 😂 😓

I need to update comments on the bash … calls though, to explain why there is 2 indeed

kk! happy for that to be in follow up PR :)

bobcatfish · 2020-01-09T18:29:25Z

/lgtm

thanks again for all your hard work @vdemeester 🙏 😭 🙇‍♀

bobcatfish · 2020-01-09T18:29:36Z

/approve

tekton-robot · 2020-01-09T18:29:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bobcatfish

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~~ [bobcatfish]

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

tekton-robot requested a review from bobcatfish December 19, 2019 15:36

tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 19, 2019

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

tekton-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Dec 19, 2019

imjasonh changed the title ~~Migrate PipelineResource in there own package… 📦~~ Migrate PipelineResource in their own package… 📦 Dec 19, 2019

vdemeester mentioned this pull request Dec 19, 2019

Add TaskRun to v1alpha2 🎋 #1725

Merged

3 tasks

bobcatfish reviewed Dec 20, 2019

View reviewed changes

vdemeester force-pushed the move-resource-separate-package branch from fe89773 to 1ffb260 Compare December 23, 2019 10:22

tekton-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 23, 2019

vdemeester force-pushed the move-resource-separate-package branch 3 times, most recently from 494f369 to fe3b432 Compare December 23, 2019 11:26

vdemeester force-pushed the move-resource-separate-package branch from fe3b432 to cd085c2 Compare December 23, 2019 11:27

vdemeester force-pushed the move-resource-separate-package branch from cd085c2 to a2467c4 Compare December 24, 2019 08:44

vdemeester force-pushed the move-resource-separate-package branch from a2467c4 to 0c4f9cd Compare December 24, 2019 09:00

vdemeester force-pushed the move-resource-separate-package branch from 0c4f9cd to 3dde233 Compare December 24, 2019 09:16

vdemeester force-pushed the move-resource-separate-package branch from 3dde233 to c9d4361 Compare December 24, 2019 09:21

vdemeester force-pushed the move-resource-separate-package branch from c9d4361 to 4d617cb Compare December 24, 2019 11:59

vdemeester mentioned this pull request Dec 24, 2019

Fail gracefully if tekton APIs are not available redhat-developer/vscode-tekton#1

Closed

vdemeester force-pushed the move-resource-separate-package branch from 4d617cb to 3600fe0 Compare January 9, 2020 13:22

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 9, 2020

vdemeester force-pushed the move-resource-separate-package branch from 3600fe0 to 30b421b Compare January 9, 2020 13:50

bobcatfish reviewed Jan 9, 2020

View reviewed changes

tekton-robot assigned bobcatfish Jan 9, 2020

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 9, 2020

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 9, 2020

tekton-robot merged commit fdbe724 into tektoncd:master Jan 9, 2020

vdemeester deleted the move-resource-separate-package branch January 10, 2020 08:16

vdemeester mentioned this pull request Jan 10, 2020

Proposal: Change API Group for triggers tektoncd/triggers#332

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate PipelineResource in their own package… 📦 #1773

Migrate PipelineResource in their own package… 📦 #1773

vdemeester commented Dec 19, 2019 •

edited

Loading

tekton-robot commented Dec 19, 2019

vdemeester commented Dec 19, 2019

bobcatfish left a comment

bobcatfish Dec 20, 2019

bobcatfish Dec 20, 2019

vdemeester commented Dec 20, 2019

bobcatfish commented Dec 20, 2019

vdemeester commented Dec 23, 2019

tekton-robot commented Dec 23, 2019

tekton-robot commented Dec 23, 2019

tekton-robot commented Dec 24, 2019

tekton-robot commented Dec 24, 2019

tekton-robot commented Dec 24, 2019

tekton-robot commented Dec 24, 2019

tekton-robot commented Dec 24, 2019

tekton-robot commented Jan 9, 2020

vdemeester commented Jan 9, 2020

vdemeester commented Jan 9, 2020

tekton-robot commented Jan 9, 2020

bobcatfish Jan 9, 2020

vdemeester Jan 9, 2020

vdemeester Jan 9, 2020

bobcatfish Jan 9, 2020

bobcatfish commented Jan 9, 2020

bobcatfish commented Jan 9, 2020

tekton-robot commented Jan 9, 2020

+              		})
+              	}
+              }

Migrate PipelineResource in their own package… 📦 #1773

Migrate PipelineResource in their own package… 📦 #1773

Conversation

vdemeester commented Dec 19, 2019 • edited Loading

Changes

Submitter Checklist

Reviewer Notes

tekton-robot commented Dec 19, 2019

vdemeester commented Dec 19, 2019

bobcatfish left a comment

Choose a reason for hiding this comment

bobcatfish Dec 20, 2019

Choose a reason for hiding this comment

bobcatfish Dec 20, 2019

Choose a reason for hiding this comment

vdemeester commented Dec 20, 2019

bobcatfish commented Dec 20, 2019

vdemeester commented Dec 23, 2019

tekton-robot commented Dec 23, 2019

tekton-robot commented Dec 23, 2019

tekton-robot commented Dec 24, 2019

tekton-robot commented Dec 24, 2019

tekton-robot commented Dec 24, 2019

tekton-robot commented Dec 24, 2019

tekton-robot commented Dec 24, 2019

tekton-robot commented Jan 9, 2020

vdemeester commented Jan 9, 2020

vdemeester commented Jan 9, 2020

tekton-robot commented Jan 9, 2020

bobcatfish Jan 9, 2020

Choose a reason for hiding this comment

vdemeester Jan 9, 2020

Choose a reason for hiding this comment

vdemeester Jan 9, 2020

Choose a reason for hiding this comment

bobcatfish Jan 9, 2020

Choose a reason for hiding this comment

bobcatfish commented Jan 9, 2020

bobcatfish commented Jan 9, 2020

tekton-robot commented Jan 9, 2020

vdemeester commented Dec 19, 2019 •

edited

Loading