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

Evaluate kubebuilder for controllers #19

Closed
bobcatfish opened this issue Sep 8, 2018 · 2 comments
Closed

Evaluate kubebuilder for controllers #19

bobcatfish opened this issue Sep 8, 2018 · 2 comments

Comments

@bobcatfish
Copy link
Collaborator

Expected Behavior

Though our controllers (e.g. pipeline run controller) don't do much, we should be happy with how they are built and prepared to move forward with the skeletons we do have as we start actually implementing functionality.

Actual Behavior

We used kubebuilder to bootstrap our controllers which allowed us to get up and moving pretty quickly. However the controllers it generates depend on a lot of code in controller-tools and controller-runtime.

We should compare a controller creating using these libs to one implemented from scratch (e.g. the knative/serving controllers) and make sure we want to keep using kubebuilder in the long run.

Additional Info

n/a

@bobcatfish bobcatfish added this to To do in Pipeline CRD via automation Sep 8, 2018
@aaron-prindle aaron-prindle self-assigned this Sep 11, 2018
@aaron-prindle aaron-prindle moved this from To do to In progress in Pipeline CRD Sep 17, 2018
bobcatfish referenced this issue in bobcatfish/pipeline Sep 17, 2018
Add to the main README - however note that since we only have PR based
tests right now, if a PR fails, our badges will go red which isn't
really fair to us or the submitter - but we can improve this in #52.

Added a go report card link too - looks like our linting could use some
improvement but we should see how #19 goes (maybe switching out
kubebuilder) before we start following up on all of these.

Also adding some details about code reviews after taking with @tejal29
and @aaron-prindle, specifically leaving the reviews up for a bit longer
than we have been to try to give more folks a chance to comment. Also
added a link to some useful Go coding standards.
bobcatfish referenced this issue in bobcatfish/pipeline Sep 17, 2018
Add to the main README - however note that since we only have PR based
tests right now, if a PR fails, our badges will go red which isn't
really fair to us or the submitter - but we can improve this in #52.

Added a go report card link too - looks like our linting could use some
improvement but we should see how #19 goes (maybe switching out
kubebuilder) before we start following up on all of these.

Also adding some details about code reviews after taking with @tejal29
and @aaron-prindle, specifically leaving the reviews up for a bit longer
than we have been to try to give more folks a chance to comment. Also
added a link to some useful Go coding standards.
bobcatfish referenced this issue in bobcatfish/pipeline Sep 17, 2018
Add to the main README - however note that since we only have PR based
tests right now, if a PR fails, our badges will go red which isn't
really fair to us or the submitter - but we can improve this in #52.

Added a go report card link too - looks like our linting could use some
improvement but we should see how #19 goes (maybe switching out
kubebuilder) before we start following up on all of these.

Also adding some details about code reviews after taking with @tejal29
and @aaron-prindle, specifically leaving the reviews up for a bit longer
than we have been to try to give more folks a chance to comment. Also
added a link to some useful Go coding standards.

Fixed a couple typos too.
bobcatfish referenced this issue in bobcatfish/pipeline Sep 18, 2018
Add to the main README - however note that since we only have PR based
tests right now, if a PR fails, our badges will go red which isn't
really fair to us or the submitter - but we can improve this in #52.

Added a go report card link too - looks like our linting could use some
improvement but we should see how #19 goes (maybe switching out
kubebuilder) before we start following up on all of these.

Also adding some details about code reviews after taking with @tejal29
and @aaron-prindle, specifically leaving the reviews up for a bit longer
than we have been to try to give more folks a chance to comment. Also
added a link to some useful Go coding standards.

Fixed a couple typos too.
knative-prow-robot pushed a commit that referenced this issue Sep 18, 2018
Add to the main README - however note that since we only have PR based
tests right now, if a PR fails, our badges will go red which isn't
really fair to us or the submitter - but we can improve this in #52.

Added a go report card link too - looks like our linting could use some
improvement but we should see how #19 goes (maybe switching out
kubebuilder) before we start following up on all of these.

Also adding some details about code reviews after taking with @tejal29
and @aaron-prindle, specifically leaving the reviews up for a bit longer
than we have been to try to give more folks a chance to comment. Also
added a link to some useful Go coding standards.

Fixed a couple typos too.
@bobcatfish bobcatfish added this to the Mid October Demo milestone Sep 21, 2018
@bobcatfish
Copy link
Collaborator Author

We have decided not to use kubebuilder for now (removing it in #57). Here is a summary of our findings (also authored by @aaron-prindle).

Kubebuilder

Pros

Cons

  • We cannot use the Knative implementation to guide us
  • There aren’t examples of controllers using v1 kubebuilder style (the samples use the v0 style)
  • In v1 of kubebuilder, there are no client libraries generated that can be compatible with existing libs such as client-go
    • Instead the generated controller operates at a “higher level” and has all of the type watching patched through in the controller-runtime library kubebuilder provides
      • This could be bad though as it is unclear how external/3rd-party people will be able to use client libraries as they are not generated (e.g. to create a client to access these types, for example like the knative end to end tests)
    • i.e it doesn’t seem to generate everything that update-codegen does

Imitating knative controllers and using knative/pkg

Pros

Uses common/documented k8s pattern of client library for types (i.e. libs generated by update-codegen which are compatible with libs like client-go)

Cons

knative/pkg has no documentation so it’s pretty hard to say what’s in there @_@ though there are generated go docs.

Pipeline CRD automation moved this from In progress to Done Sep 22, 2018
@bobcatfish
Copy link
Collaborator Author

After a huge delay, following here with some info that came up in #build re. a question from @dprotaso:

In v1 of kubebuilder, there are no client libraries generated that can be compatible with existing libs such as client-go

Question:

I don’t understand what it means to be compatible with the client-go lib?

Answer:

It means to be able to use the client-go libs with the CRDs and controllers, for example in our integration tests we're going to use the libs generated by updated-codegen.sh:
https://github.com/knative/build-pipeline/blob/9b3b398e909f231fb1a9cb5250bc8b94736e71a5/test/clients.go#L51

Which provide functions for like basic CRUD operations such as List:
https://github.com/knative/build-pipeline/blob/9b3b398e909f231fb1a9cb5250bc8b94736e71a5/test/pipeline_test.go#L48

Also functionality like parsing your local kubeconfig to for configuration etc.

But kubebuilder does plan to support client-go type functionality in the long run so we will want to revisit this, I think kubernetes-sigs/kubebuilder#403 is the relevant ticket.

chmouel pushed a commit to chmouel/tektoncd-pipeline that referenced this issue Apr 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Pipeline CRD
  
Done
Development

No branches or pull requests

2 participants