Switch to kpack #87

scothis · 2019-09-17T20:33:14Z

from https://github.com/pivotal/kpack/tree/master/pkg/apis/build/v1alpha1

codecov-io · 2019-09-17T20:52:48Z

Codecov Report

Merging #87 into master will decrease coverage by 2.9%.
The diff coverage is 0%.

@@            Coverage Diff            @@
##           master     #87      +/-   ##
=========================================
- Coverage    4.95%   2.05%   -2.91%     
=========================================
  Files          46      45       -1     
  Lines        2742    2774      +32     
=========================================
- Hits          136      57      -79     
- Misses       2604    2717     +113     
+ Partials        2       0       -2

Impacted Files	Coverage Δ
pkg/apis/build/v1alpha1/shared_types.go	`0% <ø> (ø)`	⬆️
pkg/controllers/build/application_controller.go	`0% <0%> (ø)`	⬆️
pkg/controllers/build/container_controller.go	`0% <0%> (ø)`	⬆️
...kg/controllers/build/clusterbuilders_controller.go	`0% <0%> (ø)`
pkg/controllers/build/function_controller.go	`0% <0%> (ø)`	⬆️
pkg/apis/build/v1alpha1/function_lifecycle.go	`0% <0%> (ø)`	⬆️
pkg/apis/build/v1alpha1/application_lifecycle.go	`0% <0%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd602bc...e4bd1ee. Read the comment docs.

sbawaska · 2019-09-17T23:19:44Z

pkg/apis/thirdparty/kpack/build/v1alpha1/groupversion_info.go

+
+// Package v1 contains API Schema definitions for the kpack v1alpha1 API group
+//
+// This API group is a forked subset of https://github.com/pivotal/kpack/tree/master/pkg/apis/build/v1alpha1


I know this was a problem for knative since the dependencies there were very large. Is that true with kpack as well?

Yeah, did the same for keda, but not sure I understand the rationale, either wrt size or version:

size: only code actually used is linked

versions: if our dependency depends on a different version of say a k8s API, that itself is versioned in a different package (assuming compatibility semantics post-alpha, agreed).

The primary reason for this is to avoid version hell latter on, not just with system, but also with anything that consumes system (like the cli, or other tools). There were many times where upgrading Knative was difficult not because of API changes, but because of the dependency graph. In once case, we ended up with a version deadlock because Knative Serving and Knative Build had different dependencies.

By importing the types into our project we have a clean dependency graph that is centrally managed and can be tested. Since we're only including the structs that represent the public API, these are unlikely to change in significant ways absent an API version bump (of course alphas will be alphas).

When we introduce a dependency like Keda or kpack, we're not linking against it's runtime, we're consuming it's CRD types. If these projects (or us) were not using go, we'd have to take the same action of creating structs for the types. I'm avoiding the artificial shortcut that incidentally adds massive dependency graph complexity.

Like it or not, the Kubernetes go APIs are not perfectly backwards compatible. They do make breaking changes. We should pick the version of the Kubernetes APIs we want to consume.

If you need third party validation of this approach, consider Knative and Istio

scothis · 2019-09-18T18:05:57Z

This PR now also includes controllers for Applications and Functions to be backed by kpack

scothis · 2019-09-18T19:09:57Z

This is implemented against kpack 0.0.4 (which is pending release). One gotcha is that it requires builders to implement buildpacks lifecycle 0.4. The current function builder uses 0.3 and is incompatible (functions will build and publish images, but the status will fail to update).

sbawaska · 2019-09-18T20:41:16Z

pkg/controllers/build/application_controller.go


 	// check if status has changed before updating, unless requeued
 	if !result.Requeue && !equality.Semantic.DeepEqual(application.Status, originalApplication.Status) {
 		// update status
-		log.Info("updating application status", "application", application.Name,


why not have additional logging may be at debug level?

We need to address logging. I tend to do better by thinking about a component of the system in a cross cutting fashion. (Address logging for all controllers at once) rather than address is piecemeal with gap in one controller but not another.

There are other similar concerns, like tests that I'd like to defer until we get basic functionality in place and then address comprehensively.

sbawaska · 2019-09-18T20:42:52Z

pkg/controllers/build/application_controller.go

@@ -101,86 +97,30 @@ func (r *ApplicationReconciler) reconcile(ctx context.Context, log logr.Logger,
 		}
 		return ctrl.Result{}, err
 	}
-	log.V(1).Info("resolved target image", "application", application.Name, "image", targetImage)


keep debug level logging?

pkg/controllers/build/application_controller.go

scothis · 2019-09-27T00:52:31Z

We now have a builder that is compatible with Buildpack lifecycle 0.4, but are blocked by kpacks incompatibility with unauthenticated registries.

Ref buildpacks-community/kpack#143

from https://github.com/pivotal/kpack/tree/master/pkg/apis/build/v1alpha1

scothis requested a review from sbawaska September 17, 2019 20:33

scothis force-pushed the kpack-types branch from 2ba829b to d101ed3 Compare September 17, 2019 22:31

sbawaska reviewed Sep 17, 2019

View reviewed changes

scothis changed the title ~~Add kpack types~~ Switch to kpack Sep 18, 2019

scothis requested a review from sbawaska September 18, 2019 19:10

sbawaska reviewed Sep 18, 2019

View reviewed changes

scothis force-pushed the kpack-types branch from 466a001 to 286f35c Compare September 26, 2019 21:09

scothis force-pushed the kpack-types branch 2 times, most recently from e6cbb75 to d32defb Compare October 1, 2019 14:47

scothis added 2 commits October 1, 2019 14:27

Add kpack types

9731008

from https://github.com/pivotal/kpack/tree/master/pkg/apis/build/v1alpha1

Switch from Knative Build to kpack

d6402d2

scothis force-pushed the kpack-types branch from d32defb to d6402d2 Compare October 1, 2019 18:28

scothis added 2 commits October 1, 2019 14:47

bump fats and kpack

0eb0dea

retry all kubectl apply

e4bd1ee

scothis merged commit 91b4eaf into projectriff:master Oct 1, 2019

scothis deleted the kpack-types branch October 1, 2019 19:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch to kpack #87

Switch to kpack #87

scothis commented Sep 17, 2019 •

edited

Loading

codecov-io commented Sep 17, 2019 •

edited

Loading

sbawaska Sep 17, 2019

ericbottard Sep 18, 2019

scothis Sep 18, 2019

scothis Sep 18, 2019

scothis commented Sep 18, 2019

scothis commented Sep 18, 2019

sbawaska Sep 18, 2019

scothis Sep 18, 2019

sbawaska Sep 18, 2019

scothis commented Sep 27, 2019

Switch to kpack #87

Switch to kpack #87

Conversation

scothis commented Sep 17, 2019 • edited Loading

codecov-io commented Sep 17, 2019 • edited Loading

Codecov Report

sbawaska Sep 17, 2019

Choose a reason for hiding this comment

ericbottard Sep 18, 2019

Choose a reason for hiding this comment

scothis Sep 18, 2019

Choose a reason for hiding this comment

scothis Sep 18, 2019

Choose a reason for hiding this comment

scothis commented Sep 18, 2019

scothis commented Sep 18, 2019

sbawaska Sep 18, 2019

Choose a reason for hiding this comment

scothis Sep 18, 2019

Choose a reason for hiding this comment

sbawaska Sep 18, 2019

Choose a reason for hiding this comment

scothis commented Sep 27, 2019

scothis commented Sep 17, 2019 •

edited

Loading

codecov-io commented Sep 17, 2019 •

edited

Loading