feat: builder images map migration (#1033)

* feat: function builder images migration

* fix typos and comments

client.go

@@ -557,18 +557,6 @@ func (c *Client) Create(cfg Function) (err error) {
 		return
 	}
 
-	// TODO: Create a status structure and return it for clients to use
-	// for output, such as from the CLI.
-	if c.verbose {
-		fmt.Printf("Builder:       %s\n", f.Builder)
-		if len(f.Buildpacks) > 0 {
-			fmt.Println("Buildpacks:")
-			for _, b := range f.Buildpacks {
-				fmt.Printf("           ... %s\n", b)
-			}
-		}
-		fmt.Println("Function project created")
-	}
 	return
 }
 

client_test.go

@@ -899,8 +899,8 @@ func TestClient_Deploy_UnbuiltErrors(t *testing.T) {
 	}
 }
 
-// TestClient_New_BuildersPersisted Asserts that the client preserves user-
-// provided Builders
+// TestClient_New_BuilderImagesPersisted Asserts that the client preserves user-
+// provided Builder Images
 func TestClient_New_BuildersPersisted(t *testing.T) {
 	root := "testdata/example.com/testConfiguredBuilders" // Root from which to run the test
 	defer Using(t, root)()
@@ -910,8 +910,9 @@ func TestClient_New_BuildersPersisted(t *testing.T) {
 	f0 := fn.Function{
 		Runtime: TestRuntime,
 		Root:    root,
-		Builders: map[string]string{
-			"custom": "docker.io/example/custom",
+		BuilderImages: map[string]string{
+			"pack": "example.com/my/custom-pack-builder",
+			"s2i":  "example.com/my/custom-s2i-builder",
 		}}
 
 	// Create the Function, which should preserve custom builders
@@ -926,8 +927,8 @@ func TestClient_New_BuildersPersisted(t *testing.T) {
 	}
 
 	// Assert that our custom builders were retained
-	if !reflect.DeepEqual(f0.Builders, f1.Builders) {
-		t.Fatalf("Expected %v but got %v", f0.Builders, f1.Builders)
+	if !reflect.DeepEqual(f0.BuilderImages, f1.BuilderImages) {
+		t.Fatalf("Expected %v but got %v", f0.BuilderImages, f1.BuilderImages)
 	}
 
 	// A Default Builder(image) is not asserted here, because that is
@@ -937,41 +938,6 @@ func TestClient_New_BuildersPersisted(t *testing.T) {
 	// A builder image may also be manually specified of course.
 }
 
-// TestClient_New_BuilderDefault ensures that if a custom builder is
-// provided of name "default", this is chosen as the default builder instead
-// of the inbuilt static default.
-func TestClient_New_BuilderDefault(t *testing.T) {
-	root := "testdata/example.com/testConfiguredBuildersWithDefault" // Root from which to run the test
-	defer Using(t, root)()
-
-	builders := map[string]string{
-		"custom":  "docker.io/example/custom",
-		"default": "docker.io/example/default",
-	}
-	client := fn.New(fn.WithRegistry(TestRegistry))
-	if err := client.New(context.Background(), fn.Function{
-		Runtime:  TestRuntime,
-		Root:     root,
-		Builders: builders,
-	}); err != nil {
-		t.Fatal(err)
-	}
-	f, err := fn.NewFunction(root)
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	// Assert that our custom builder array was set
-	if !reflect.DeepEqual(f.Builders, builders) {
-		t.Fatalf("Expected %v but got %v", builders, f.Builders)
-	}
-
-	// Asser that the default is also set
-	if f.Builder != builders["default"] {
-		t.Fatalf("Expected %s but got %s", builders["default"], f.Builder)
-	}
-}
-
 // TestClient_New_BuildpacksPersisted ensures that provided buildpacks are
 // persisted on new Functions.
 func TestClient_New_BuildpacksPersisted(t *testing.T) {

cmd/build.go

@@ -100,7 +100,7 @@ func runBuild(cmd *cobra.Command, _ []string, newClient ClientFactory) (err erro
 		return
 	}
 
-	function, err := functionWithOverrides(config.Path, functionOverrides{BuilderImage: config.BuilderImage, Image: config.Image})
+	function, err := functionWithOverrides(config.Path, functionOverrides{Image: config.Image})
 	if err != nil {
 		return
 	}
@@ -166,6 +166,11 @@ func runBuild(cmd *cobra.Command, _ []string, newClient ClientFactory) (err erro
 		return
 	}
 
+	// Use the user-provided builder image, if supplied
+	if config.BuilderImage != "" {
+		function.BuilderImages[config.Builder] = config.BuilderImage
+	}
+
 	// Create a client using the registry defined in config plus any additional
 	// options provided (such as mocks for testing)
 	client, done := newClient(ClientConfig{Verbose: config.Verbose},

cmd/completion_util.go

@@ -114,8 +114,8 @@ func CompleteBuilderImageList(cmd *cobra.Command, args []string, complete string
 		return
 	}
 
-	builderImages = make([]string, 0, len(f.Builders))
-	for name := range f.Builders {
+	builderImages = make([]string, 0, len(f.BuilderImages))
+	for name := range f.BuilderImages {
 		if len(complete) == 0 {
 			builderImages = append(builderImages, name)
 			continue

cmd/root.go

@@ -168,9 +168,8 @@ func bindEnv(flags ...string) bindFunc {
 }
 
 type functionOverrides struct {
-	Image        string
-	Namespace    string
-	BuilderImage string
+	Image     string
+	Namespace string
 }
 
 // functionWithOverrides sets the namespace and image strings for the
@@ -187,7 +186,6 @@ func functionWithOverrides(root string, overrides functionOverrides) (f fn.Funct
 		src  string
 		dest *string
 	}{
-		{overrides.BuilderImage, &f.Builder},
 		{overrides.Image, &f.Image},
 		{overrides.Namespace, &f.Namespace},
 	}

function.go

@@ -64,15 +64,9 @@ type Function struct {
 	// in case build type "git" is being used
 	Git Git `yaml:"git"`
 
-	// Builder represents the CNCF Buildpack builder image for a function
-	Builder string `yaml:"builder"`
-
-	// Map containing known builders.
-	// e.g. { "jvm": "docker.io/example/quarkus-jvm-builder" }
-	Builders map[string]string `yaml:"builders"`
-
 	// BuilderImages define optional explicit builder images to use by
-	// builder implementations in leau of the in-code defaults.
+	// builder implementations in leau of the in-code defaults.  They key
+	// is the builder's short name.  For example:
 	// builderImages:
 	//   pack: example.com/user/my-pack-node-builder
 	//   s2i: example.com/user/my-s2i-node-builder
@@ -121,8 +115,8 @@ type HealthEndpoints struct {
 
 // BuildConfig defines builders and buildpacks
 type BuildConfig struct {
-	Buildpacks []string          `yaml:"buildpacks,omitempty"`
-	Builders   map[string]string `yaml:"builders,omitempty"`
+	Buildpacks    []string          `yaml:"buildpacks,omitempty"`
+	BuilderImages map[string]string `yaml:"builderImages,omitempty"`
 }
 
 // Invocation defines hints on how to accomplish a Function invocation.
@@ -169,7 +163,7 @@ func NewFunction(path string) (f Function, err error) {
 	if err != nil {
 		return
 	}
-	if err = yaml.UnmarshalStrict(bb, &f); err != nil {
+	if err = yaml.Unmarshal(bb, &f); err != nil {
 		err = formatUnmarshalError(err) // human-friendly unmarshalling errors
 		return
 	}
@@ -486,7 +480,7 @@ func hasInitializedFunction(path string) (bool, error) {
 		return false, err
 	}
 	f := Function{}
-	if err = yaml.UnmarshalStrict(bb, &f); err != nil {
+	if err = yaml.Unmarshal(bb, &f); err != nil {
 		return false, err
 	}
 	if f, err = f.Migrate(); err != nil {

function_migrations.go

@@ -1,9 +1,12 @@
 package function
 
 import (
+	"io/ioutil"
+	"path/filepath"
 	"time"
 
 	"github.com/coreos/go-semver/semver"
+	"gopkg.in/yaml.v2"
 )
 
 // Migrate applies any necessary migrations, returning a new migrated
@@ -74,13 +77,15 @@ func (f Function) Migrated() bool {
 // version for the migration if necessary)
 var migrations = []migration{
 	{"0.19.0", migrateToCreationStamp},
+	{"0.23.0", migrateToBuilderImages},
 	// New Migrations Here.
 }
 
 // Individual Migration implementations
 // ------------------------------------
 
-// migrateToCreationStamp is the initial migration which brings a Function from
+// migrateToCreationStamp
+// The initial migration which brings a Function from
 // some unknown point in the past to the point at which it is versioned,
 // migrated and includes a creation timestamp.  Without this migration,
 // instantiation of old functions will fail with a "Function at path X not
@@ -112,3 +117,73 @@ func migrateToCreationStamp(f Function, m migration) (Function, error) {
 	f.Version = m.version // Record this migration was evaluated.
 	return f, nil
 }
+
+// migrateToBuilderImages
+// Prior to this migration, 'builder' and 'builders' attributes of a Function
+// were specific to buildpack builds.  In addition, the separation of the two
+// fields was to facilitate the use of "named" inbuilt builders, which ended
+// up not being necessary.  With the addition of the S2I builder implementation,
+// it is necessary to differentiate builders for use when building via Pack vs
+// builder for use when building with S2I.  Furthermore, now that the builder
+// itself is a user-supplied parameter, the short-hand of calling builder images
+// simply "builder" is not possible, since that term more correctly refers to
+// the builder being used (S2I, pack, or some future implementation), while this
+// field very specifically refers to the image the chosen builder should use
+// (in leau of the inbuilt default).
+//
+// For an example of the situation:  the 'builder' member used to instruct the
+// system to use that builder _image_ in all cases.  This of course restricts
+// the system from being able to build with anything other than the builder
+// implementation to which that builder image applies (pack or s2i).  Further,
+// always including this value in the serialized func.yaml causes this value to
+// be pegged/immutable (without manual intervention), which hampers our ability
+// to change out the underlying builder image with future versions.
+//
+// The 'builder' and 'builders' members have therefore been removed in favor
+// of 'builderImages', which is keyed by the short name of the builder
+// implementation (currently 'pack' and 's2i').  Its existence is optional,
+// with the default value being provided in the associated builder's impl.
+// Should the value exist, this indicates the user has overridden the value,
+// or is using a fully custom language pack.
+//
+// This migration allows pre-builder-image Functions to load despite their
+// inclusion of the now removed 'builder' member.  If the user had provided
+// a customized builder image, that value is preserved as the builder image
+// for the 'pack' builder in the new version (s2i did not exist prior).
+// See associated unit tests.
+func migrateToBuilderImages(f1 Function, m migration) (Function, error) {
+	// Load the Function using pertinent parts of the previous version's schema:
+	f0Filename := filepath.Join(f1.Root, FunctionFile)
+	bb, err := ioutil.ReadFile(f0Filename)
+	if err != nil {
+		return f1, err
+	}
+	f0 := migrateToBuilderImages_previousFunction{}
+	if err = yaml.Unmarshal(bb, &f0); err != nil {
+		return f1, err
+	}
+
+	// At time of this migration, the default pack builder image for all language
+	// runtimes is:
+	defaultPackBuilderImage := "gcr.io/paketo-buildpacks/builder:base"
+
+	// If the old Function had defined something custom
+	if f0.Builder != "" && f0.Builder != defaultPackBuilderImage {
+		// carry it forward as the new pack builder image
+		if f1.BuilderImages == nil {
+			f1.BuilderImages = make(map[string]string)
+		}
+		f1.BuilderImages["pack"] = f0.Builder
+	}
+
+	// Flag f1 as having had the migration applied
+	f1.Version = m.version
+	return f1, nil
+
+}
+
+// The pertinent asspects of the Function schema prior to the builder images
+// migration.
+type migrateToBuilderImages_previousFunction struct {
+	Builder string `yaml:"builder"`
+}

function_migrations_unit_test.go

@@ -82,6 +82,48 @@ func TestMigrateToCreationStamp(t *testing.T) {
 	}
 }
 
+// TestMigrateToBuilderImages ensures that the migration which migrates
+// from "builder" and "builders" to "builderImages" is applied.  This results
+// in the attributes being removed and no errors on load of the function with
+// old schema.
+func TestMigrateToBuilderImagesDefault(t *testing.T) {
+	// Load a Function created prior to the adoption of the builder images map
+	// (was created with 'builder' and 'builders' which does not support different
+	// builder implementations.
+	root := "testdata/migrations/v0.23.0"
+
+	// Without the migration, instantiating the older Function would error
+	// because its strict unmarshalling would fail parsing the unexpected
+	// 'builder' and 'builders' members.
+	_, err := NewFunction(root)
+	if err != nil {
+		t.Fatal(err)
+	}
+}
+
+// TestMigrateToBuilderImagesCustom ensures that the migration to builderImages
+// correctly carries forward a customized value for 'builder'.
+func TestMigrateToBuilderImagesCustom(t *testing.T) {
+	// An early version of a Function which includes a customized value for
+	// the 'builder'.  This should be correctly carried forward to
+	// the namespaced 'builderImages' map as image for the "pack" builder.
+	root := "testdata/migrations/v0.23.0-customized"
+	expected := "example.com/user/custom-builder" // set in testdata func.yaml
+
+	f, err := NewFunction(root)
+	if err != nil {
+		t.Fatal(f)
+	}
+	i, ok := f.BuilderImages["pack"]
+	if !ok {
+		t.Fatal("migrated Function does not include the pack builder images")
+	}
+	if i != expected {
+		t.Fatalf("migrated Function expected builder image '%v', got '%v'", expected, i)
+	}
+
+}
+
 func latestMigrationVersion() string {
 	return migrations[len(migrations)-1].version
 }

schema/func_yaml-schema.json

@@ -29,8 +29,6 @@
 				"imageDigest",
 				"build",
 				"git",
-				"builder",
-				"builders",
 				"buildpacks",
 				"volumes",
 				"buildEnvs",
@@ -75,17 +73,6 @@
 					"$schema": "http://json-schema.org/draft-04/schema#",
 					"$ref": "#/definitions/Git"
 				},
-				"builder": {
-					"type": "string"
-				},
-				"builders": {
-					"patternProperties": {
-						".*": {
-							"type": "string"
-						}
-					},
-					"type": "object"
-				},
 				"builderImages": {
 					"patternProperties": {
 						".*": {

template.go

@@ -37,14 +37,8 @@ func (t template) Write(ctx context.Context, f *Function) error {
 	// so it's values are treated as defaults.
 	// TODO: this begs the question: should the Template's manifest.yaml actually
 	// be a partially-populated func.yaml?
-	if f.Builder == "" { // as a special first case, this default comes from itself
-		f.Builder = f.Builders["default"]
-		if f.Builder == "" { // still nothing?  then use the template
-			f.Builder = t.config.Builders["default"]
-		}
-	}
-	if len(f.Builders) == 0 {
-		f.Builders = t.config.Builders
+	if len(f.BuilderImages) == 0 {
+		f.BuilderImages = t.config.BuilderImages
 	}
 	if len(f.Buildpacks) == 0 {
 		f.Buildpacks = t.config.Buildpacks

testdata/migrations/v0.23.0-customized/func.yaml

@@ -0,0 +1,26 @@
+version: 0.0.0
+name: testfunc
+namespace: ""
+runtime: go
+registry: ""
+image: ""
+imageDigest: ""
+build: local
+git: {}
+builder: "example.com/user/custom-builder"
+builders: {}
+buildpacks:
+- paketo-buildpacks/go-dist
+- ghcr.io/boson-project/go-function-buildpack:tip
+volumes: []
+buildEnvs: []
+envs: []
+annotations: {}
+options: {}
+labels: []
+healthEndpoints:
+  liveness: /health/liveness
+  readiness: /health/readiness
+created: 2022-05-25T22:44:47.36886+09:00
+invocation:
+  format: http

testdata/migrations/v0.23.0/func.yaml

@@ -0,0 +1,26 @@
+version: 0.0.0
+name: testfunc
+namespace: ""
+runtime: go
+registry: ""
+image: ""
+imageDigest: ""
+build: local
+git: {}
+builder: ""
+builders: {}
+buildpacks:
+- paketo-buildpacks/go-dist
+- ghcr.io/boson-project/go-function-buildpack:tip
+volumes: []
+buildEnvs: []
+envs: []
+annotations: {}
+options: {}
+labels: []
+healthEndpoints:
+  liveness: /health/liveness
+  readiness: /health/readiness
+created: 2022-05-25T22:44:47.36886+09:00
+invocation:
+  format: http