Repository: knative-sandbox/kn-plugin-func
Pull request #738 (2022-01-06 15:53:29)

Revert "feat: add possibility to disable pushing of image in `deploy` command"
Reverts knative-sandbox/kn-plugin-func#736

This change hasn't been ready yet/

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/738#" title="Author self-approved">zroubalik</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)~~ [zroubalik]

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":[]} -->
codecov[bot]: # [Codecov](https://codecov.io/gh/knative-sandbox/kn-plugin-func/pull/738?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox) Report
> Merging [#738](https://codecov.io/gh/knative-sandbox/kn-plugin-func/pull/738?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox) (c82ce46) into [main](https://codecov.io/gh/knative-sandbox/kn-plugin-func/commit/4e5a5e830799b73f65aba8ee248a52bf1c643acb?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox) (4e5a5e8) will **decrease** coverage by `0.00%`.
> The diff coverage is `12.50%`.

[![Impacted file tree graph](https://codecov.io/gh/knative-sandbox/kn-plugin-func/pull/738/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/738?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     #738      +/-   ##
==========================================
- Coverage   41.56%   41.56%   -0.01%     
==========================================
  Files          42       42              
  Lines        4102     4083      -19     
==========================================
- Hits         1705     1697       -8     
+ Misses       2144     2135       -9     
+ Partials      253      251       -2     
```


| [Impacted Files](https://codecov.io/gh/knative-sandbox/kn-plugin-func/pull/738?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox) | Coverage Δ | |
|---|---|---|
| [cmd/deploy.go](https://codecov.io/gh/knative-sandbox/kn-plugin-func/pull/738/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox#diff-Y21kL2RlcGxveS5nbw==) | `16.17% <0.00%> (+0.13%)` | :arrow_up: |
| [client.go](https://codecov.io/gh/knative-sandbox/kn-plugin-func/pull/738/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox#diff-Y2xpZW50Lmdv) | `65.08% <33.33%> (+0.06%)` | :arrow_up: |
| [cmd/build.go](https://codecov.io/gh/knative-sandbox/kn-plugin-func/pull/738/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox#diff-Y21kL2J1aWxkLmdv) | `58.08% <100.00%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/knative-sandbox/kn-plugin-func/pull/738?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/738?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox). Last update [4e5a5e8...c82ce46](https://codecov.io/gh/knative-sandbox/kn-plugin-func/pull/738?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).

lance: /lgtm

Patch:
diff --git a/client.go b/client.go
index 1575dce7..10188eec 100644
--- a/client.go
+++ b/client.go
@@ -424,12 +424,6 @@ func (c *Client) New(ctx context.Context, cfg Function) (err error) {
 		return
 	}
 
-	// Push the produced function image
-	c.progressListener.Increment("Pushing container image to registry")
-	if err = c.Push(ctx, f.Root); err != nil {
-		return
-	}
-
 	// Deploy the initialized Function, returning its publicly
 	// addressible name for possible registration.
 	c.progressListener.Increment("Deploying Function to cluster")
@@ -608,6 +602,10 @@ func (c *Client) Deploy(ctx context.Context, path string) (err error) {
 		return ErrNotBuilt
 	}
 
+	if err = c.Push(ctx, &f); err != nil {
+		return
+	}
+
 	// Deploy a new or Update the previously-deployed Function
 	c.progressListener.Increment("Deploying function to the cluster")
 	result, err := c.deployer.Deploy(ctx, f)
@@ -719,23 +717,8 @@ func (c *Client) Emit(ctx context.Context, endpoint string) error {
 }
 
 // Push the image for the named service to the configured registry
-func (c *Client) Push(ctx context.Context, path string) (err error) {
-	go func() {
-		<-ctx.Done()
-		c.progressListener.Stopping()
-	}()
-
-	f, err := NewFunction(path)
-	if err != nil {
-		return
-	}
-
-	if !f.Built() {
-		return ErrNotBuilt
-	}
-
-	c.progressListener.Increment("Pushing function image to the registry")
-	imageDigest, err := c.pusher.Push(ctx, f)
+func (c *Client) Push(ctx context.Context, f *Function) (err error) {
+	imageDigest, err := c.pusher.Push(ctx, *f)
 	if err != nil {
 		return
 	}
diff --git a/cmd/build.go b/cmd/build.go
index 16c897a5..c61a7a75 100644
--- a/cmd/build.go
+++ b/cmd/build.go
@@ -51,6 +51,7 @@ func newBuildClient(cfg buildConfig) (*fn.Client, error) {
 
 	return fn.New(
 		fn.WithBuilder(builder),
+		// fn.WithPusher(pusher),
 		pusherOption,
 		fn.WithProgressListener(listener),
 		fn.WithRegistry(cfg.Registry), // for image name when --image not provided
@@ -200,7 +201,7 @@ func runBuild(cmd *cobra.Command, _ []string, clientFn buildClientFn) (err error
 
 	err = client.Build(cmd.Context(), config.Path)
 	if err == nil && config.Push {
-		err = client.Push(cmd.Context(), config.Path)
+		err = client.Push(cmd.Context(), &function)
 	}
 	return
 }
diff --git a/cmd/deploy.go b/cmd/deploy.go
index 54495ecb..6fbb71c2 100644
--- a/cmd/deploy.go
+++ b/cmd/deploy.go
@@ -27,23 +27,19 @@ func init() {
 
 func newDeployClient(cfg deployConfig) (*fn.Client, error) {
 	listener := progress.New()
+
 	builder := buildpacks.NewBuilder()
 
-	pusherOption := fn.WithPusher(nil)
-	if cfg.Push {
-		credentialsProvider := creds.NewCredentialsProvider(
-			creds.WithPromptForCredentials(newPromptForCredentials()),
-			creds.WithPromptForCredentialStore(newPromptForCredentialStore()),
-			creds.WithTransport(cfg.Transport))
-		pusher, err := docker.NewPusher(
-			docker.WithCredentialsProvider(credentialsProvider),
-			docker.WithProgressListener(listener),
-			docker.WithTransport(cfg.Transport))
-		if err != nil {
-			return nil, err
-		}
-		pusher.Verbose = cfg.Verbose
-		pusherOption = fn.WithPusher(pusher)
+	credentialsProvider := creds.NewCredentialsProvider(
+		creds.WithPromptForCredentials(newPromptForCredentials()),
+		creds.WithPromptForCredentialStore(newPromptForCredentialStore()),
+		creds.WithTransport(cfg.Transport))
+	pusher, err := docker.NewPusher(
+		docker.WithCredentialsProvider(credentialsProvider),
+		docker.WithProgressListener(listener),
+		docker.WithTransport(cfg.Transport))
+	if err != nil {
+		return nil, err
 	}
 
 	deployer, err := knative.NewDeployer(cfg.Namespace)
@@ -53,12 +49,13 @@ func newDeployClient(cfg deployConfig) (*fn.Client, error) {
 
 	listener.Verbose = cfg.Verbose
 	builder.Verbose = cfg.Verbose
+	pusher.Verbose = cfg.Verbose
 	deployer.Verbose = cfg.Verbose
 
 	return fn.New(
 		fn.WithProgressListener(listener),
 		fn.WithBuilder(builder),
-		pusherOption,
+		fn.WithPusher(pusher),
 		fn.WithDeployer(deployer),
 		fn.WithRegistry(cfg.Registry), // for deriving name when no --image value
 		fn.WithVerbose(cfg.Verbose),
@@ -103,7 +100,6 @@ kn func deploy --image quay.io/myuser/myfunc -n myns
 	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", "", "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("build", "b", true, "Build the image before deploying (Env: $FUNC_BUILD)")
-	cmd.Flags().BoolP("push", "u", true, "Attempt to push the function image to registry before deploying.")
 	setPathFlag(cmd)
 	setNamespaceFlag(cmd)
 
@@ -202,12 +198,6 @@ func runDeploy(cmd *cobra.Command, _ []string, clientFn deployClientFn) (err err
 		}
 	}
 
-	if config.Push {
-		if err := client.Push(cmd.Context(), config.Path); err != nil {
-			return err
-		}
-	}
-
 	return client.Deploy(cmd.Context(), config.Path)
 
 	// NOTE: Namespace is optional, default is that used by k8s client
@@ -299,9 +289,6 @@ type deployConfig struct {
 	// Build the associated Function before deploying.
 	Build bool
 
-	// Push function image to the registry before deploying.
-	Push bool
-
 	// Envs passed via cmd to be added/updated
 	EnvToUpdate *util.OrderedMap
 
@@ -326,7 +313,6 @@ func newDeployConfig(cmd *cobra.Command) (deployConfig, error) {
 		Verbose:     viper.GetBool("verbose"), // defined on root
 		Confirm:     viper.GetBool("confirm"),
 		Build:       viper.GetBool("build"),
-		Push:        viper.GetBool("push"),
 		EnvToUpdate: envToUpdate,
 		EnvToRemove: envToRemove,
 	}, nil


Test patch:
diff --git a/cmd/build_test.go b/cmd/build_test.go
index 00d9be66..56b733dc 100644
--- a/cmd/build_test.go
+++ b/cmd/build_test.go
@@ -144,58 +144,31 @@ created: 2009-11-10 23:00:00`,
 
 func Test_newBuildClient(t *testing.T) {
 	tests := []struct {
-		name         string
-		cfg          buildConfig
-		fileContents string
-		succeed      bool
+		name    string
+		cfg     buildConfig
+		succeed bool
 	}{
 		{
 			name: "push flag set to false avoids pusher instanciation",
 			cfg: buildConfig{
 				Push: false,
 			},
-			fileContents: `name: test-func
-runtime: go
-image: registry.io/foo/bar:latest
-imageDigest: sha256:1111111111111111111111
-created: 2009-11-10 23:00:00`,
 			succeed: false,
 		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-
-			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)
-			}
-
 			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(), tempDir)
+			err = client.Push(context.TODO(), &fn.Function{})
 			if err != nil {
 				t.Error(err)
 			}