No kubebuilder #66

aaron-prindle · 2018-09-25T00:44:48Z

This PR removes the kubebuilder dependency and framework from knative/build-pipeline, opting to instead use the hack/update-codegen.sh scripts approach to generating CRD libraries. The controller and directory structure were also updated to more closely mirror knative/build. Resource was temporarily renamed StandardResource as it collided with a method Register in pkg/apis/pipeline/v1alpha1/register.go.

nader-ziada

I understand this is a work in progress, but wanted to comment to the parts I saw that we part of build and I don't think should not be here

nader-ziada · 2018-09-25T13:02:27Z

DEVELOPMENT.md

+# Next, make sure that you have no builds or build templates in your current namespace:
+kubectl delete builds --all
+kubectl delete buildtemplates --all
+kubectl delete clusterbuildtemplates --all



This cleanup of builds, buildtemplates and clusterbuildtemplates is not applicable here, should be cleaning up pipeline things

nader-ziada · 2018-09-25T13:02:57Z

DEVELOPMENT.md


-To make changes to these CRDs, you will probably interact with:
+```


The way the build tests run might not be applicable here

nader-ziada · 2018-09-25T13:03:37Z

README.md

@@ -1,142 +1,51 @@
-# Pipeline CRD
+# Knative build



should be build-pipeline

nader-ziada · 2018-09-25T13:04:27Z

README.md

all this readme is about build, we need the one about build-pipeline that already existed

nader-ziada · 2018-09-25T13:05:14Z

cmd/controller/main.go

+	logger = logger.With(zap.String(logkey.ControllerType, logLevelKey))
+
+	logger.Info("Starting the Build Controller")
+


I'm guessing this should be pipeline controller

The Build controller is actually a bit old-fashioned in Knative terms, and doesn't use some of the new best practices available in knative/pkg, like the reconciler and webhook validation stuff. It might be better to base pipelines' controller on serving or eventing, until build can get updated to the latest-and-greatest.

For a super skeletal example to start from using the knative/pkg goodies check out https://github.com/mattmoor/warm-image.

This doesn't leverage things like the webhook (which Build hasn't yet adopted), but it shows the Reconciler bits.

There are a skeleton implementation in build but its not fully completed yet. Serving is a better example

I'm going to leave this as is for now and refactor to using the new controller interfaces/libs in another PR

isn't the point of this PR to refactor the controllers interfaces libs to the new style? Just that it will be harder to refactor when other work gets added to this structure

isn't the point of this PR to refactor the controllers interfaces libs to the new style?

This particular PR is about removing kubebuilder, but I agree that we want the controller code to settle in the style we want to continue with. Since this PR is so huge, I think we should go ahead with it as-is, then have a subsequent PR to update to the new style.

Hopefully I'm not underestimating the differences between this and the new style, but from what I can tell it seems like folks can go ahead and get unblocked if we merge this, then it's just a few changes to update to the latest.

(As part of updating to the latest I'd also like to write some docs about it b/c I haven't been able to find any.)

ok, I agree this PR is getting to big to follow, we can fix up whatever needs changes after.

nader-ziada · 2018-09-25T13:16:03Z

cmd/creds-init/main.go

+	defer logger.Sync()
+
+	builders := []credentials.Builder{dockercreds.NewBuilder(), gitcreds.NewBuilder()}
+	for _, c := range builders {


builders is what the build project uses to execute the builds, I'm expecting it will be a little different here

removed creds-init for now

nader-ziada · 2018-09-25T13:16:43Z

cmd/logs/main.go

+func main() {
+	flag.Parse()
+	if len(flag.Args()) != 1 {
+		log.Fatalf("Usage: %s [-n NAMESPACE] BUILD-NAME\n", os.Args[0])


Pipeline name instead of build name

Do we need the logs package at all for now? I'd prefer to add it only when/if we find a use for it.

removed cmd/logs for now

nader-ziada · 2018-09-25T13:17:42Z

config/300-build.yaml

+# limitations under the License.
+apiVersion: apiextensions.k8s.io/v1beta1
+kind: CustomResourceDefinition
+metadata:


the build CRD should not be part of the build-pipeline project

Currently, I believe the idea is to have Tasks create builds from the buildSpec field in a Task. It is unclear if installing knative/build should be a dependency for knative/build-pipeline or we should import pieces of knative/build in knative/build-pipeline (build controller, etc.) and manage those pieces that way. I am open to either. Currently, the build CRD is in config/ as the installation method is ko apply -f config and Build CRD is a dependency for the Task CRD.

I think it would be better as a dependency, similar to what serving has. This way we would have less duplication and can easily pick up changes in the build project

knative/build is now a requirement to install knative/build-pipeline, removed related yamls and controllers from knative/build-pipeline

nader-ziada · 2018-09-25T13:17:58Z

config/300-buildtemplate.yaml

+# See the License for the specific language governing permissions and
+# limitations under the License.
+apiVersion: apiextensions.k8s.io/v1beta1
+kind: CustomResourceDefinition


the buildTemplate CRD should not be part of the build-pipeline project

nader-ziada · 2018-09-25T13:18:18Z

config/300-clusterbuildtemplate.yaml

+apiVersion: apiextensions.k8s.io/v1beta1
+kind: CustomResourceDefinition
+metadata:
+  name: clusterbuildtemplates.pipeline.knative.dev


the clusterBuildTemplate CRD should not be part of the build-pipeline project

bobcatfish

When you're ready to go, plz make sure to update the commit message(s) to:

explain why we're doing this (https://github.com/knative/build-pipeline/blob/master/CONTRIBUTING.md#commit-messages)
include "fixes #57"

/me focusing on the important things first 😎

bobcatfish · 2018-09-25T17:13:25Z

Another request for the commit message: listing everything that existed in the kubebuilder based iteration that was removed and why (e.g. "existing tests had to be removed because X")

bobcatfish

I think a lot of this was covered by @pivotal-nader-ziada already but here's a list of the immediate things I've noticed:

This PR clobbers our existing:
- issue-template.md
- README.md
- CONTRIBUTING.md
- DEVELOPMENT.md - most of what got replaced here is great and super useful, but we lost some stuff from our existing file (e.g. getting started, where's the code) and gained some stuff that doesn't quite make sense (e.g. running the integration tests)
- pkg/apis/pipeline/v1alpha1/resource_types.go is gone for some reason

It also removes all of our existing tests - is it possible to keep sanity_test.go (including the dependency on kubebuilder) around, or does that only work with the kubebuilder based controllers?

What is cover.out?

I think we should remove for now:

AUTHORS
config/*.yaml which duplicate types from knative/build that we don't use (e.g. 300-build, 300-build-template, i think there are probably more like imagecache, etc.)
cmd/creds-init, cmd/git-init (until we need them, maybe @pivotal-nader-ziada and @shashwathi will be re-adding these or have a different opinion)
cmd/logs - unless we need it? it's not clear to me waht this is

I'd like to get a review from someone like @imjasonh specifically of the contents of config/*.yaml, it's not 100% clear to me which of those we need (e.g. is everything in the cluster role required)

bobcatfish · 2018-09-25T17:30:24Z

I like how the controllers are empty of logic for now! I think that's a great starting place so we can unblock @pivotal-nader-ziada, @shashwathi and our other issues.

re. the better controller implementations that @imjasonh and @mattmoor mentioned we can work from (thanks for the pointer in the right direction!! ❤️ ), I suggest we go ahead and commit the controllers as we have them in this PR, then update them in the next PR (so we don't have to do too much in this PR, there's so much already). But feel free to disregard if you'd prefer @aaron-prindle

tejal29 · 2018-09-25T17:58:13Z

👍 Looking forward for this!

shashwathi · 2018-09-25T18:18:57Z

cmd/logs

That is used for tailing build logs. For now I would vote for removing this component and add the feature when we user requests it.

cmd/creds-init, cmd/git-init

Yes I vote for removing these as well. @pivotal-nader-ziada let me know if you feel otherwise.

git init & creds init leftovers

config/999-cache.yaml file
Not to include git-init, creds-init
config/controller.yaml file
Controller does not need init flags.

Updating docs

I will also echo everybody's thought on not adding this change.

bobcatfish · 2018-09-26T22:04:33Z

I think this is almost there! I'm making a few more tweaks.

This PR removes the kubebuilder dependency and framework from knative/build-pipeline, opting to instead use the hack/update-codegen.sh scripts approach to generating CRD libraries. The controller and directory structure were also updated to more closely mirror knative/build. Resource was temporarily renamed StandardResource as it collided with a method Register in pkg/apis/pipeline/v1alpha1/register.go. Currently tests are stubbed out since all of the existing tests were kubebuilder specific - we'll have to add more! Updates developer docs to include how to interact with this repo now that we've switched styles. Fixes #57

In our first iteration of the API, we were using `v1beta1` as the version. We bumped this to `v1alpha1` while removing kubebuilder but didn't update it everywhere. Also removed extra references to git-init and creds-init (at the moment we aren't building these from anything in this repo) Co-authored-by: Tejal Desai <tejaldesai@google.com>

bobcatfish · 2018-09-26T23:29:13Z

Okay I think we are ready! drumroll

bobcatfish · 2018-09-26T23:29:39Z

/lgtm
/approve
/meow space

knative-prow-robot · 2018-09-26T23:29:41Z

@bobcatfish:

In response to this:

/lgtm
/approve
/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.

knative-prow-robot · 2018-09-26T23:29:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aaron-prindle, 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

bobcatfish · 2018-09-26T23:31:43Z

This has mostly addressed #57 , less the changes suggested by @mattmoor and @imjasonh

* Replace kubebuilder with knative/build style controllers This PR removes the kubebuilder dependency and framework from knative/build-pipeline, opting to instead use the hack/update-codegen.sh scripts approach to generating CRD libraries. The controller and directory structure were also updated to more closely mirror knative/build. Resource was temporarily renamed StandardResource as it collided with a method Register in pkg/apis/pipeline/v1alpha1/register.go. Currently tests are stubbed out since all of the existing tests were kubebuilder specific - we'll have to add more! Updates developer docs to include how to interact with this repo now that we've switched styles. Fixes #57 * Fix resource version In our first iteration of the API, we were using `v1beta1` as the version. We bumped this to `v1alpha1` while removing kubebuilder but didn't update it everywhere. Also removed extra references to git-init and creds-init (at the moment we aren't building these from anything in this repo) Co-authored-by: Tejal Desai <tejaldesai@google.com>

knative-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Sep 25, 2018

knative-prow-robot requested review from bobcatfish and tejal29 September 25, 2018 00:45

nader-ziada suggested changes Sep 25, 2018

View reviewed changes

bobcatfish reviewed Sep 25, 2018

View reviewed changes

bobcatfish modified the milestone: Mid October Demo Sep 26, 2018

aaron-prindle and others added 2 commits September 26, 2018 16:09

knative-prow-robot assigned bobcatfish Sep 26, 2018

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 26, 2018

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 26, 2018

knative-prow-robot merged commit 01853c6 into tektoncd:master Sep 26, 2018

This was referenced Sep 27, 2018

Dep script #26

Closed

Use knative/serving style controllers #71

Closed

S-hayeon mentioned this pull request Feb 9, 2022

Fix long CRD tmax-cloud/cicd-operator#332

Open


		To make changes to these CRDs, you will probably interact with:
		```

		logger = logger.With(zap.String(logkey.ControllerType, logLevelKey))

		logger.Info("Starting the Build Controller")

No kubebuilder #66

No kubebuilder #66

Conversation

aaron-prindle commented Sep 25, 2018

nader-ziada left a comment

Choose a reason for hiding this comment

nader-ziada Sep 25, 2018 • edited by aaron-prindle Loading

Choose a reason for hiding this comment

nader-ziada Sep 25, 2018 • edited by aaron-prindle Loading

Choose a reason for hiding this comment

nader-ziada Sep 25, 2018 • edited by aaron-prindle Loading

Choose a reason for hiding this comment

nader-ziada Sep 25, 2018 • edited by aaron-prindle Loading

Choose a reason for hiding this comment

nader-ziada Sep 25, 2018 • edited by aaron-prindle Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nader-ziada Sep 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nader-ziada Sep 25, 2018 • edited by aaron-prindle Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaron-prindle Sep 26, 2018 • edited Loading

Choose a reason for hiding this comment

nader-ziada Sep 25, 2018 • edited by aaron-prindle Loading

Choose a reason for hiding this comment

nader-ziada Sep 25, 2018 • edited by aaron-prindle Loading

Choose a reason for hiding this comment

bobcatfish left a comment

Choose a reason for hiding this comment

bobcatfish commented Sep 25, 2018

bobcatfish left a comment

Choose a reason for hiding this comment

bobcatfish commented Sep 25, 2018

tejal29 commented Sep 25, 2018

shashwathi commented Sep 25, 2018

bobcatfish commented Sep 26, 2018

bobcatfish commented Sep 26, 2018

bobcatfish commented Sep 26, 2018

knative-prow-robot commented Sep 26, 2018

knative-prow-robot commented Sep 26, 2018

bobcatfish commented Sep 26, 2018

nader-ziada Sep 25, 2018 •

edited by aaron-prindle

Loading

nader-ziada Sep 25, 2018 •

edited by aaron-prindle

Loading

nader-ziada Sep 25, 2018 •

edited by aaron-prindle

Loading

nader-ziada Sep 25, 2018 •

edited by aaron-prindle

Loading

nader-ziada Sep 25, 2018 •

edited by aaron-prindle

Loading

nader-ziada Sep 26, 2018 •

edited

Loading

nader-ziada Sep 25, 2018 •

edited by aaron-prindle

Loading

aaron-prindle Sep 26, 2018 •

edited

Loading

nader-ziada Sep 25, 2018 •

edited by aaron-prindle

Loading

nader-ziada Sep 25, 2018 •

edited by aaron-prindle

Loading