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

feat: s2i builder with preliminary node support #923

Merged
merged 24 commits into from
Apr 5, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 20 additions & 2 deletions buildpacks/builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package buildpacks
import (
"bytes"
"context"
"errors"
"fmt"
"io"
"os"
Expand All @@ -21,6 +20,12 @@ import (
"github.com/buildpacks/pack/pkg/logging"
)

// DefaultBuilderImages for Pack builders indexed by Runtime Language
var DefaultBuilderImages = map[string]string{
"node": "gcr.io/paketo-buildpacks/builder:base",
"go": "gcr.io/paketo-buildpacks/builder:base",
Copy link
Member

Choose a reason for hiding this comment

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

This won't really work for Go functions though, will it? They need the scaffolding that is in the buildpack as well, right?

Copy link
Member Author

@lkingland lkingland Apr 5, 2022

Choose a reason for hiding this comment

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

It is my understanding that the base is the same, but the buildpack builder will still apply any buildpacks defined, which include ghcr.io/boson-project/go-function-buildpack:tip, which has the scaffolding. The S2I builder, when updated to support Go, will of course ignore that and use its own method, which I presume will be a single builder image that includes everything required.

Copy link
Member

Choose a reason for hiding this comment

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

It is my understanding that the base is the same, but the buildpack builder will still apply any buildpacks defined, which include ghcr.io/boson-project/go-function-buildpack:tip, which has the scaffolding.

Yes, this is the case, if it is explicitly provided, for example in a func.yaml file in the buildpacks field. Is that what you are expecting? Without that, the scaffold isn't there.

}

//Builder holds the configuration that will be passed to
//Buildpack builder
type Builder struct {
Expand All @@ -46,7 +51,10 @@ func (builder *Builder) Build(ctx context.Context, f fn.Function) (err error) {
packBuilder = pb
}
} else {
return errors.New("no buildpack configured for function")
packBuilder, err = defaultBuilderImage(f)
if err != nil {
return
}
}

// Build options for the pack client.
Expand Down Expand Up @@ -139,6 +147,16 @@ func (builder *Builder) Build(ctx context.Context, f fn.Function) (err error) {
return
}

// defaultBuilderImage for the given function based on its runtime, or an
// error if no default is defined for the given runtime.
func defaultBuilderImage(f fn.Function) (string, error) {
v, ok := DefaultBuilderImages[f.Runtime]
if !ok {
return "", fmt.Errorf("Pack builder has no default builder image specified for the '%v' language runtime. Please provide one.", f.Runtime)
}
return v, nil
}

// hack this makes stdout non-closeable
type stdoutWrapper struct {
impl io.Writer
Expand Down
2 changes: 1 addition & 1 deletion client.go
Original file line number Diff line number Diff line change
Expand Up @@ -644,7 +644,6 @@ func (c *Client) printBuildActivity(ctx context.Context) {
}
i := 0
ticker := time.NewTicker(10 * time.Second)
defer ticker.Stop()
go func() {
for {
select {
Expand All @@ -654,6 +653,7 @@ func (c *Client) printBuildActivity(ctx context.Context) {
i = i % len(m)
case <-ctx.Done():
c.progressListener.Stopping()
ticker.Stop()
return
}
}
Expand Down
12 changes: 6 additions & 6 deletions client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -900,8 +900,7 @@ func TestClient_Deploy_UnbuiltErrors(t *testing.T) {
}

// TestClient_New_BuildersPersisted Asserts that the client preserves user-
// provided Builders on the Function configuration with the internal default
// if not provided.
// provided Builders
func TestClient_New_BuildersPersisted(t *testing.T) {
root := "testdata/example.com/testConfiguredBuilders" // Root from which to run the test
defer Using(t, root)()
Expand Down Expand Up @@ -931,10 +930,11 @@ func TestClient_New_BuildersPersisted(t *testing.T) {
t.Fatalf("Expected %v but got %v", f0.Builders, f1.Builders)
}

// But that the default exists
if f1.Builder == "" {
t.Fatal("Expected default builder to be set")
}
// A Default Builder(image) is not asserted here, because that is
// the responsibility of the Builder(type) being used to build the Function.
// The builder (Buildpack,s2i, etc) will have a default builder image for
// the given Function or will error that the Function is not supported.
// A builder image may also be manually specified of course.
}

// TestClient_New_BuilderDefault ensures that if a custom builder is
Expand Down
52 changes: 36 additions & 16 deletions cmd/build.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@ import (
"github.com/ory/viper"
"github.com/spf13/cobra"

"knative.dev/kn-plugin-func/buildpacks"
"knative.dev/kn-plugin-func/s2i"

fn "knative.dev/kn-plugin-func"
)

Expand Down Expand Up @@ -40,17 +43,18 @@ and the image name is stored in the configuration file.
{{.Name}} build --builder cnbs/sample-builder:bionic
`,
SuggestFor: []string{"biuld", "buidl", "built"},
PreRunE: bindEnv("image", "path", "builder", "registry", "confirm", "push"),
PreRunE: bindEnv("image", "path", "builder", "registry", "confirm", "push", "builder-image"),
}

cmd.Flags().StringP("builder", "b", "", "Buildpack builder, either an as a an image name or a mapping name.\nSpecified value is stored in func.yaml for subsequent builds.")
cmd.Flags().StringP("builder", "b", "pack", "builder to use when creating the underlying image. Currently supported builders are 'pack' and 's2i'.")
cmd.Flags().StringP("builder-image", "", "", "builder image, either an as a an image name or a mapping name.\nSpecified value is stored in func.yaml for subsequent builds. ($FUNC_BUILDER_IMAGE)")
Copy link
Member

Choose a reason for hiding this comment

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

It strikes me as clumsy that there is both a builder and build-image flag. I wonder if it should rather be something like strategy and build-image (or build), where strategy is a fixed set (buildpack, s2i, etc) and build-image or build is an image address.

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. We should try to consolidate this a little bit. I wouldn't block this PR on this though. But it is something we should try to tackle as soon as possilbe. To provide consistency and more clarity for users.

Copy link
Member Author

Choose a reason for hiding this comment

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

In order to not block this PR, I have opened a new issue to discuss better flag naming: #942

cmd.Flags().BoolP("confirm", "c", false, "Prompt to confirm all configuration options (Env: $FUNC_CONFIRM)")
cmd.Flags().StringP("image", "i", "", "Full image name in the form [registry]/[namespace]/[name]:[tag] (optional). This option takes precedence over --registry (Env: $FUNC_IMAGE)")
cmd.Flags().StringP("registry", "r", GetDefaultRegistry(), "Registry + namespace part of the image to build, ex 'quay.io/myuser'. The full image name is automatically determined based on the local directory name. If not provided the registry will be taken from func.yaml (Env: $FUNC_REGISTRY)")
cmd.Flags().BoolP("push", "u", false, "Attempt to push the function image after being successfully built")
setPathFlag(cmd)

if err := cmd.RegisterFlagCompletionFunc("builder", CompleteBuilderList); err != nil {
if err := cmd.RegisterFlagCompletionFunc("builder", CompleteBuilderImageList); err != nil {
fmt.Println("internal: error while calling RegisterFlagCompletionFunc: ", err)
}

Expand Down Expand Up @@ -89,7 +93,7 @@ func runBuild(cmd *cobra.Command, _ []string, newClient ClientFactory) (err erro
return
}

function, err := functionWithOverrides(config.Path, functionOverrides{Builder: config.Builder, Image: config.Image})
function, err := functionWithOverrides(config.Path, functionOverrides{BuilderImage: config.BuilderImage, Image: config.Image})
if err != nil {
return
}
Expand Down Expand Up @@ -144,15 +148,22 @@ func runBuild(cmd *cobra.Command, _ []string, newClient ClientFactory) (err erro
config.Registry = ""
}

// TODO(lkingland): The below deferred options gathering is what will
// re-enable the addition of alternative implementations of the Builder,
// unblocking PR https://github.com/knative-sandbox/kn-plugin-func/pull/842
// the implementation of which will be inserted here.
// Choose a builder based on the value of the --builder flag
var builder fn.Builder
if config.Builder == "pack" {
builder = buildpacks.NewBuilder(config.Verbose)
} else if config.Builder == "s2i" {
builder = s2i.NewBuilder(config.Verbose)
} else {
err = errors.New("unrecognized builder")
return
}

// 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},
fn.WithRegistry(config.Registry))
fn.WithRegistry(config.Registry),
fn.WithBuilder(builder))
defer done()

err = client.Build(cmd.Context(), config.Path)
Expand Down Expand Up @@ -184,18 +195,27 @@ type buildConfig struct {
// Confirm: confirm values arrived upon from environment plus flags plus defaults,
// with interactive prompting (only applicable when attached to a TTY).
Confirm bool

// Builder is the name of the subsystem that will complete the underlying
// build (Pack, s2i, remote pipeline, etc). Currently ad-hoc rather than
// an enumerated field. See the Client constructory for logic.
Builder string

// BuilderImage is the image (name or mapping) to use for building. Usually
// set automatically.
BuilderImage string
}

func newBuildConfig() buildConfig {
return buildConfig{
Image: viper.GetString("image"),
Path: viper.GetString("path"),
Registry: viper.GetString("registry"),
Verbose: viper.GetBool("verbose"), // defined on root
Confirm: viper.GetBool("confirm"),
Builder: viper.GetString("builder"),
Push: viper.GetBool("push"),
Image: viper.GetString("image"),
Path: viper.GetString("path"),
Registry: viper.GetString("registry"),
Verbose: viper.GetBool("verbose"), // defined on root
Confirm: viper.GetBool("confirm"),
Builder: viper.GetString("builder"),
BuilderImage: viper.GetString("builder-image"),
Push: viper.GetBool("push"),
}
}

Expand Down
2 changes: 1 addition & 1 deletion cmd/completion_util.go
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ func CompleteRegistryList(cmd *cobra.Command, args []string, toComplete string)
return
}

func CompleteBuilderList(cmd *cobra.Command, args []string, complete string) (strings []string, directive cobra.ShellCompDirective) {
func CompleteBuilderImageList(cmd *cobra.Command, args []string, complete string) (strings []string, directive cobra.ShellCompDirective) {
directive = cobra.ShellCompDirectiveError

var (
Expand Down
8 changes: 4 additions & 4 deletions cmd/root.go
Original file line number Diff line number Diff line change
Expand Up @@ -168,9 +168,9 @@ func bindEnv(flags ...string) bindFunc {
}

type functionOverrides struct {
Image string
Namespace string
Builder string
Image string
Namespace string
BuilderImage string
}

// functionWithOverrides sets the namespace and image strings for the
Expand All @@ -187,7 +187,7 @@ func functionWithOverrides(root string, overrides functionOverrides) (f fn.Funct
src string
dest *string
}{
{overrides.Builder, &f.Builder},
{overrides.BuilderImage, &f.Builder},
{overrides.Image, &f.Image},
{overrides.Namespace, &f.Namespace},
}
Expand Down
8 changes: 8 additions & 0 deletions docker/docker_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,14 @@ import (
"github.com/docker/docker/client"
)

// NewClient creates a new docker client.
// reads the DOCKER_HOST envvar but it may or may not return it as dockerHost.
// - For local connection (unix socket and windows named pipe) it returns the
// DOCKER_HOST directly.
// - For ssh connections it reads the DOCKER_HOST from the ssh remote.
// - For TCP connections it returns "" so it defaults in the remote (note that
// one should not be use client.DefaultDockerHost in this situation). This is
// needed beaus of TCP+tls connections.
func NewClient(defaultHost string) (dockerClient client.CommonAPIClient, dockerHost string, err error) {
var _url *url.URL

Expand Down
6 changes: 5 additions & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ require (
github.com/alecthomas/jsonschema v0.0.0-20210526225647-edb03dcab7bc
github.com/buildpacks/pack v0.24.0
github.com/cloudevents/sdk-go/v2 v2.5.0
github.com/containers/image/v5 v5.10.6
github.com/containers/image/v5 v5.19.1
github.com/coreos/go-semver v0.3.0
github.com/docker/cli v20.10.12+incompatible
github.com/docker/docker v20.10.12+incompatible
Expand All @@ -23,6 +23,7 @@ require (
github.com/hinshun/vt10x v0.0.0-20180809195222-d55458df857c
github.com/mitchellh/go-homedir v1.1.0
github.com/opencontainers/image-spec v1.0.3-0.20220114050600-8b9d41f48198
github.com/openshift/source-to-image v1.3.1
github.com/ory/viper v1.7.5
github.com/pkg/errors v0.9.1
github.com/spf13/cobra v1.3.0
Expand All @@ -45,6 +46,9 @@ require (
)

replace (
// update docker to be compatible with version used by pack and removes invalid pseudo-version
github.com/openshift/source-to-image => github.com/boson-project/source-to-image v1.3.2
lance marked this conversation as resolved.
Show resolved Hide resolved

// Pin k8s.io dependencies to align with Knative and Tekton needs
k8s.io/api => k8s.io/api v0.22.5
k8s.io/apimachinery => k8s.io/apimachinery v0.22.5
Expand Down
Loading