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

**POTENTIAL BREAKING CHANGES** RFC: Rename the CustomBuilder Resource. Remove the Builder Resource. #439

Merged

Conversation

matthewmcnew
Copy link
Collaborator

@matthewmcnew matthewmcnew commented Jul 14, 2020

@matthewmcnew matthewmcnew changed the title WIP - Remove Builders in favor of CustomBuilders RFC: Rename the CustomBuilder Resource. Remove the Builder Resource. Jul 14, 2020
@matthewmcnew matthewmcnew marked this pull request as ready for review July 14, 2020 20:32
Copy link
Contributor

@tylerphelan tylerphelan left a comment

Choose a reason for hiding this comment

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

Overall I would lean to approving this RFC. But would feel better if we had a strong mitigation for the transition from pack to kpack.

Edit: approving because I think the benefits of simplicity outweigh the loss of pack build && kubectl create builder -f pack-builder.yaml.

@@ -0,0 +1,34 @@
Today, kpack has a complicated hierarchy of builder resources (Builder & ClusterBuilder & CustomBuilder & CustomClusterBuilder). Navigating this hierarchy is hard to explain and has consistently been confusing in early feedback. This is complicated by the recommended kpack experience being "CustomBuilders" when it appears that the obvious default is "Builder" or "ClusterBuilder".

The existing "Builder" relies on a polling mechanism to monitor an external Builder for buildpack and stack updates. External polling mechanisms in core kpack types have been a source of confusion that contradicts the pure declarative philosophy outlined in the [kpack monitors rfc](https://github.com/pivotal/kpack/pull/433).
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 that I think kpack monitors cover the use case of Builders

The Builder could be renamed to something like "PreBuilt" Builder. This will allow users to continue utilizing external Builders. However, this approach does not resolve the underlying problems with the Builder concept and may continue to encourage users to use the PreBuiltBuilder.


## Risks:
Copy link
Contributor

Choose a reason for hiding this comment

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

This has been brought up by @djoyahoy – one risk is that the Builder is a Cloud Native Buildpacks concept (however not a spec concept) and is an artifact that can be created and used by the Cloud Native Buildpacks pack cli project.

This means the builders created by pack can be used in kpack and makes builders a natural transition path from using pack cli to using kpack. Removing the Builder resource could make this transition tougher.

Copy link
Contributor

@sukhil-suresh sukhil-suresh Jul 17, 2020

Choose a reason for hiding this comment

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

If we do switch to renaming CustomBuilder > Builder, it would end up with the term Builder meaning two different things (as pointed out by @tylerphelan) in the CNB context and kpack context. And, considering kpack is a CNB platform implementation, this may not be ideal?

I do agree that CustomBuilder usage is the best experience for a kpack user.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This means the builders created by pack can be used in kpack and makes builders a natural transition path from using pack cli to using kpack. Removing the Builder resource could make this transition tougher.

I am hopeful that our kpack cli will be able to help make the transition from pack to kpack smoother and easier for new users.

I also would conjecture that it is more important to optimize for onboarding with (Custom)Builders in kpack than optimizing for onboarding in general.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If we do switch to renaming CustomBuilder > Builder, it would end up with the term Builder meaning two different things (as pointed out by @tylerphelan) in the CNB context and kpack context. And, considering kpack is a CNB platform implementation, this may not be ideal?

I'll need to think about this a bit more but, it is not quite two different things because the (Custom)Builder concept in kpack is broadly equivalent to pack create-builder and kpack (Custom)Builder image are usable as a builder with pack. So, it is more that pack created builders are not directly useable with kpack.

@@ -0,0 +1,34 @@
Today, kpack has a complicated hierarchy of builder resources (Builder & ClusterBuilder & CustomBuilder & CustomClusterBuilder). Navigating this hierarchy is hard to explain and has consistently been confusing in early feedback. This is complicated by the recommended kpack experience being "CustomBuilders" when it appears that the obvious default is "Builder" or "ClusterBuilder".
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that the overhead and confusion of having two resources is a significant concern.

@thephw
Copy link
Contributor

thephw commented Jul 16, 2020

I remember circling back a bunch of times re-reading the docs before settling on what is presently CustomBuilder. I am a little curious about the use-case for the ClusterCustomBuilder. I sort of went back and forth on which to use, but opted for a CustomBuilder given I had no use case to expose that functionality cluster-wide.

If we can't ponder a valid use case for the ClusterCustomerBuilder it could be another decision point to axe to simplify initial adoption.

@tylerphelan tylerphelan changed the title RFC: Rename the CustomBuilder Resource. Remove the Builder Resource. **POTENTIAL BREAKING CHANGES** RFC: Rename the CustomBuilder Resource. Remove the Builder Resource. Jul 17, 2020
@matthewmcnew
Copy link
Collaborator Author

That is a good question @thephw. I think the thought is that a Custom(Cluster)Builders are useable in a multi-namespace/multi-tentant environment where a cluster administer would like to create and manage a builder for multiple different namespaces.

It seems like our docs should recommend starting with namespaced resources. Do you think you would ever use kpack in a multi-namespace environment?

@thephw
Copy link
Contributor

thephw commented Jul 21, 2020

@matthewmcnew We really use the build artifacts (built images) across namespaces, but not the whole kpack build process itself. We have the build process isolated since buildpacks need source access and we build images for multiple tenants of the service. In our usage, namespaces and other k8s security policies help avoid having as many attack vectors for multi-tenant builds.

@matthewmcnew
Copy link
Collaborator Author

@thephw Doest that mean all kpack images are in one namespace?

@mgibson1121 mgibson1121 self-requested a review July 24, 2020 14:11
@elenorebastian elenorebastian self-assigned this Jul 24, 2020
@elenorebastian elenorebastian self-requested a review July 24, 2020 16:01
@sampeinado
Copy link
Contributor

sampeinado commented Jul 24, 2020

I like this idea a lot, but I would like to also raise the possibility that we remove ClusterBuilder as an independent thing. Couldn't the scope of the builder (namespace or cluster) be part of its configuration, a property of a Builder, instead of making it a separate entity with its own name?

@sampeinado
Copy link
Contributor

sampeinado commented Jul 24, 2020

This may be implicitly part of the RFC and I'm missing it, but I understood the intention of the static "Builder" to partly be a convenience for new users who just wanted to learn the tool and weren't particularly picky about how their app got built (i.e. didn't need to customize or optimize the build process). How does this change affect this use case? Will there still be a default builder to get started with?

@matthewmcnew
Copy link
Collaborator Author

I like this idea a lot, but I would like to also raise the possibility that we remove ClusterBuilder as an independent thing. Couldn't the scope of the builder (namespace or cluster) be part of its configuration, a property of a Builder, instead of making it a separate entity with its own name?

@sampeinado A resource has a scope (Namespace/Cluster), so it is not possible to for this to be configured in the resource's spec.

@matthewmcnew
Copy link
Collaborator Author

This may be implicitly part of the RFC and I'm missing it, but I understood the intention of the static "Builder" to partly be a convenience for new users who just wanted to learn the tool and weren't particularly picky about how their app got built (i.e. didn't need to customize or optimize the build process). How does this change affect this use case? Will there still be a default builder to get started with?

@sampeinado kpack will provide new tooling to make it easy to easily get setup. This is likely possible with an import functionality on the kpack cli that can automatically setup the necessary resources.

@matthewmcnew
Copy link
Collaborator Author

Merging. This will be released in the 0.1.0 release of kpack. Please submit a future RFC if you would like to take kpack in a different direction.

@matthewmcnew matthewmcnew merged commit 2b97f7b into buildpacks-community:master Aug 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants