Repository: knative-sandbox/kn-plugin-func
Pull request #681 (2021-11-29 05:04:21)

feat: add flag to push image at the end of a successful build
<!-- Thanks for sending a pull request! -->

# Changes

<!-- 
Describe your changes here- ideally you can get that description straight from
your descriptive commit message(s)! 

- :gift: Add new feature
- :bug: Fix bug
- :broom: Update or clean up current behavior
- :wastebasket: Remove feature or internal logic
-->

- First approach for #676 

<!--
In addition, categorize the changes you're making using the "/kind" Prow command, example:

Supported kinds are: api-change, bug, cleanup, deprecation, removal, documentation, enhancement, performance

-->

<!--
*Automatically closes linked issue when PR is merged.
Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`.
_If PR is about `failing-tests or flakes`, please post the related issues/tests in a comment and do not use `Fixes`_*
-->
Currently have some debug statements in the code because I'm having an issue where the `push` flag is available to the command but it's not getting picked up by viper

<!-- Please include the 'why' behind your changes if no issue exists -->



knative-prow-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative-sandbox/kn-plugin-func/pull/681#" title="Author self-approved">shinigambit</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **lkingland** after the PR has been reviewed.
You can assign the PR to them by writing `/assign @lkingland` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative-sandbox%2Fkn-plugin-func).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/knative-sandbox/kn-plugin-func/blob/main/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["lkingland"]} -->
codecov[bot]: # [Codecov](https://codecov.io/gh/knative-sandbox/kn-plugin-func/pull/681?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox) Report
> Merging [#681](https://codecov.io/gh/knative-sandbox/kn-plugin-func/pull/681?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox) (59bb439) into [main](https://codecov.io/gh/knative-sandbox/kn-plugin-func/commit/ccf00152be0ceba1794267f8e03a09cb32fee514?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox) (ccf0015) will **decrease** coverage by `0.19%`.
> The diff coverage is `12.90%`.

[![Impacted file tree graph](https://codecov.io/gh/knative-sandbox/kn-plugin-func/pull/681/graphs/tree.svg?width=650&height=150&src=pr&token=da1B9bMsQ9&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox)](https://codecov.io/gh/knative-sandbox/kn-plugin-func/pull/681?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox)

```diff
@@            Coverage Diff             @@
##             main     #681      +/-   ##
==========================================
- Coverage   38.03%   37.84%   -0.20%     
==========================================
  Files          42       42              
  Lines        3917     3942      +25     
==========================================
+ Hits         1490     1492       +2     
- Misses       2225     2248      +23     
  Partials      202      202              
```


| [Impacted Files](https://codecov.io/gh/knative-sandbox/kn-plugin-func/pull/681?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox) | Coverage Δ | |
|---|---|---|
| [cmd/build.go](https://codecov.io/gh/knative-sandbox/kn-plugin-func/pull/681/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox#diff-Y21kL2J1aWxkLmdv) | `23.30% <4.00%> (-4.22%)` | :arrow_down: |
| [client.go](https://codecov.io/gh/knative-sandbox/kn-plugin-func/pull/681/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox#diff-Y2xpZW50Lmdv) | `61.67% <50.00%> (+0.16%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/knative-sandbox/kn-plugin-func/pull/681?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/knative-sandbox/kn-plugin-func/pull/681?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox). Last update [ccf0015...59bb439](https://codecov.io/gh/knative-sandbox/kn-plugin-func/pull/681?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox).

salaboy: @shinigambit this is looking good.. what is going on with the e2e tests? It will be good not to change the default behaviour of build or even deploy with this change. 
salaboy: /retest
salaboy: @shinigambit let's ping @lance and @zroubalik .. can you guys confirm that these are real issues/failing tests? I am a bit afraid that this might be an issue with the platform more than actual issues with the code. 
salaboy: I don't have enough rights to re-trigger the tests
lance: I tried re-running the integration tests and they failed again. I don't think this is a problem with the test environment. Have you tried running integration tests locally? `make test-integration`
shinigambit: I've found one integration test that fails but it doesn't happen all the time it's `TestNew` in `client_int_test.go` which I ran as follows
`go test -v -run ^TestNew$ client_int_test.go`
When it passes it shows the following message:
```
https://testnew/
Removing Knative Service: testnew
--- PASS: TestNew (17.52s)
PASS
```
When it fails it shows the following message:
```
Waiting for Knative Service to become ready
    client_int_test.go:91: knative deployer failed to wait for the Knative Service to become ready: RevisionFailed: Revision "testnew-00001" failed with message: Unable to fetch image "localhost:5000/func/testnew:latest": failed to resolve image to digest: Get "https://localhost:5000/v2/": dial tcp [::1]:5000: connect: connection refused; Get "http://localhost:5000/v2/": dial tcp [::1]:5000: connect: connection refused.
--- FAIL: TestNew (20.32s)
FAIL
```
I'm running this in an actual Ubuntu machine in GCP (not sure if this is relevant)
The success/fail ratio seems to be exactly 50/50 with a success happening right after a fail and vice-versa @lance @salaboy 
shinigambit: Ok it seems that when the test fails it leaves the `testnew` service lingering after the test execution, somehow that makes the test pass the next time it's executed and on a successful execution the service gets deleted and the cycle repeats
shinigambit: My bad, had to pass the function arg as a pointer to fix it @lance @salaboy 
knative-prow-robot: @shinigambit: PR needs rebase.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>
knative-prow-robot: New changes are detected. LGTM label has been removed.
knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative-sandbox/kn-plugin-func/pull/681#pullrequestreview-822080904" title="LGTM">lance</a>*, *<a href="https://github.com/knative-sandbox/kn-plugin-func/pull/681#" title="Author self-approved">shinigambit</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative-sandbox%2Fkn-plugin-func).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/knative-sandbox/kn-plugin-func/blob/main/OWNERS)~~ [lance]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
salaboy: /lgtm
salaboy: Hopefully, we can get merged soon.. so I can update the tekton tasks before I leave on holidays. 
zroubalik: /unhold

Patch:
diff --git a/client.go b/client.go
index a02754f6..21b77987 100644
--- a/client.go
+++ b/client.go
@@ -593,15 +593,7 @@ func (c *Client) Deploy(ctx context.Context, path string) (err error) {
 		return ErrNotBuilt
 	}
 
-	// Push the image for the named service to the configured registry
-	imageDigest, err := c.pusher.Push(ctx, f)
-	if err != nil {
-		return
-	}
-
-	// Record the Image Digest pushed.
-	f.ImageDigest = imageDigest
-	if err = f.Write(); err != nil {
+	if err = c.Push(ctx, &f); err != nil {
 		return
 	}
 
@@ -715,6 +707,18 @@ func (c *Client) Emit(ctx context.Context, endpoint string) error {
 	return c.emitter.Emit(ctx, endpoint)
 }
 
+// Push the image for the named service to the configured registry
+func (c *Client) Push(ctx context.Context, f *Function) (err error) {
+	imageDigest, err := c.pusher.Push(ctx, *f)
+	if err != nil {
+		return
+	}
+
+	// Record the Image Digest pushed.
+	f.ImageDigest = imageDigest
+	return f.Write()
+}
+
 // DEFAULTS
 // ---------
 
diff --git a/cmd/build.go b/cmd/build.go
index a60231d7..c20d206d 100644
--- a/cmd/build.go
+++ b/cmd/build.go
@@ -11,6 +11,7 @@ import (
 
 	fn "knative.dev/kn-plugin-func"
 	"knative.dev/kn-plugin-func/buildpacks"
+	"knative.dev/kn-plugin-func/docker"
 	"knative.dev/kn-plugin-func/progress"
 )
 
@@ -24,11 +25,31 @@ func newBuildClient(cfg buildConfig) (*fn.Client, error) {
 	builder := buildpacks.NewBuilder()
 	listener := progress.New()
 
+	pusherOption := fn.WithPusher(nil)
+	if cfg.Push {
+		credentialsProvider := docker.NewCredentialsProvider(
+			newCredentialsCallback(),
+			docker.CheckAuth,
+			newChooseHelperCallback(),
+		)
+		pusher, err := docker.NewPusher(
+			docker.WithCredentialsProvider(credentialsProvider),
+			docker.WithProgressListener(listener),
+		)
+
+		if err != nil {
+			return nil, err
+		}
+		pusher.Verbose = cfg.Verbose
+		pusherOption = fn.WithPusher(pusher)
+	}
 	builder.Verbose = cfg.Verbose
 	listener.Verbose = cfg.Verbose
 
 	return fn.New(
 		fn.WithBuilder(builder),
+		// fn.WithPusher(pusher),
+		pusherOption,
 		fn.WithProgressListener(listener),
 		fn.WithRegistry(cfg.Registry), // for image name when --image not provided
 		fn.WithVerbose(cfg.Verbose)), nil
@@ -64,7 +85,7 @@ kn func build
 kn func build --builder cnbs/sample-builder:bionic
 `,
 		SuggestFor: []string{"biuld", "buidl", "built"},
-		PreRunE:    bindEnv("image", "path", "builder", "registry", "confirm"),
+		PreRunE:    bindEnv("image", "path", "builder", "registry", "confirm", "push"),
 	}
 
 	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.")
@@ -72,6 +93,7 @@ kn func build --builder cnbs/sample-builder:bionic
 	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("path", "p", cwd(), "Path to the project directory (Env: $FUNC_PATH)")
 	cmd.Flags().StringP("registry", "r", "", "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")
 
 	if err := cmd.RegisterFlagCompletionFunc("builder", CompleteBuilderList); err != nil {
 		fmt.Println("internal: error while calling RegisterFlagCompletionFunc: ", err)
@@ -170,7 +192,11 @@ func runBuild(cmd *cobra.Command, _ []string, clientFn buildClientFn) (err error
 		return err
 	}
 
-	return client.Build(cmd.Context(), config.Path)
+	err = client.Build(cmd.Context(), config.Path)
+	if err == nil && config.Push {
+		err = client.Push(cmd.Context(), &function)
+	}
+	return
 }
 
 type buildConfig struct {
@@ -206,6 +232,7 @@ func newBuildConfig() buildConfig {
 		Verbose:  viper.GetBool("verbose"), // defined on root
 		Confirm:  viper.GetBool("confirm"),
 		Builder:  viper.GetString("builder"),
+		Push:     viper.GetBool("push"),
 	}
 }
 
diff --git a/cmd/root.go b/cmd/root.go
index 8257d971..74ddec34 100644
--- a/cmd/root.go
+++ b/cmd/root.go
@@ -156,7 +156,7 @@ func cwd() (cwd string) {
 // bindFunc which conforms to the cobra PreRunE method signature
 type bindFunc func(*cobra.Command, []string) error
 
-// bindEnv returns a bindFunc that binds env vars to the namd flags.
+// bindEnv returns a bindFunc that binds env vars to the named flags.
 func bindEnv(flags ...string) bindFunc {
 	return func(cmd *cobra.Command, args []string) (err error) {
 		for _, flag := range flags {


Test patch:
diff --git a/cmd/build_test.go b/cmd/build_test.go
index 199a46a2..56b733dc 100644
--- a/cmd/build_test.go
+++ b/cmd/build_test.go
@@ -1,9 +1,13 @@
 package cmd
 
 import (
+	"context"
+	"fmt"
 	"io/ioutil"
+	"os"
 	"testing"
 
+	"github.com/ory/viper"
 	fn "knative.dev/kn-plugin-func"
 	"knative.dev/kn-plugin-func/mock"
 )
@@ -49,3 +53,125 @@ created: 2021-01-01T00:00:00+00:00
 		t.Fatal("Expected error")
 	}
 }
+
+func Test_runBuild(t *testing.T) {
+	tests := []struct {
+		name         string
+		pushFlag     bool
+		fileContents string
+		shouldBuild  bool
+		shouldPush   bool
+		wantErr      bool
+	}{
+		{
+			name:     "push flag triggers push after build",
+			pushFlag: true,
+			fileContents: `name: test-func
+runtime: go
+created: 2009-11-10 23:00:00`,
+			shouldBuild: true,
+			shouldPush:  true,
+		},
+		{
+			name:     "push flag with failing push",
+			pushFlag: true,
+			fileContents: `name: test-func
+runtime: go
+created: 2009-11-10 23:00:00`,
+			shouldBuild: true,
+			shouldPush:  true,
+			wantErr:     true,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			mockPusher := mock.NewPusher()
+			failPusher := &mock.Pusher{
+				PushFn: func(f fn.Function) (string, error) {
+					return "", fmt.Errorf("push failed")
+				},
+			}
+			mockBuilder := mock.NewBuilder()
+			cmd := NewBuildCmd(func(bc buildConfig) (*fn.Client, error) {
+				pusher := mockPusher
+				if tt.wantErr {
+					pusher = failPusher
+				}
+				return fn.New(
+					fn.WithBuilder(mockBuilder),
+					fn.WithPusher(pusher),
+				), nil
+			})
+
+			tempDir, err := os.MkdirTemp("", "func-tests")
+			if err != nil {
+				t.Fatalf("temp dir couldn't be created %v", err)
+			}
+			t.Log("tempDir created:", tempDir)
+			t.Cleanup(func() {
+				os.RemoveAll(tempDir)
+			})
+
+			fullPath := tempDir + "/func.yaml"
+			tempFile, err := os.Create(fullPath)
+			if err != nil {
+				t.Fatalf("temp file couldn't be created %v", err)
+			}
+			_, err = tempFile.WriteString(tt.fileContents)
+			if err != nil {
+				t.Fatalf("file content was not written %v", err)
+			}
+
+			cmd.SetArgs([]string{"--path=" + tempDir})
+			viper.SetDefault("push", tt.pushFlag)
+			viper.SetDefault("registry", "docker.io/tigerteam")
+
+			err = cmd.Execute()
+			if tt.wantErr != (err != nil) {
+				t.Errorf("Wanted error %v but actually got %v", tt.wantErr, err)
+			}
+
+			if mockBuilder.BuildInvoked != tt.shouldBuild {
+				t.Errorf("Build execution expected: %v but was actually %v", tt.shouldBuild, mockBuilder.BuildInvoked)
+			}
+
+			if tt.shouldPush != (mockPusher.PushInvoked || failPusher.PushInvoked) {
+				t.Errorf("Push execution expected: %v but was actually mockPusher invoked: %v failPusher invoked %v", tt.shouldPush, mockPusher.PushInvoked, failPusher.PushInvoked)
+			}
+		})
+	}
+}
+
+func Test_newBuildClient(t *testing.T) {
+	tests := []struct {
+		name    string
+		cfg     buildConfig
+		succeed bool
+	}{
+		{
+			name: "push flag set to false avoids pusher instanciation",
+			cfg: buildConfig{
+				Push: false,
+			},
+			succeed: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			client, err := newBuildClient(tt.cfg)
+			if err != nil {
+				t.Error(err)
+			}
+			defer func() {
+				if r := recover(); r != nil && tt.succeed {
+					t.Errorf("expected function call to succeed %v, got actually %v", tt.succeed, r)
+				}
+			}()
+			err = client.Push(context.TODO(), &fn.Function{})
+			if err != nil {
+				t.Error(err)
+			}
+		})
+	}
+}