Repository: knative-sandbox/kn-plugin-func
Pull request #1213 (2022-09-05 10:13:43)

fix: on cluster build - populate image name if not set
Signed-off-by: Zbynek Roubalik <zroubalik@gmail.com>

The recent changes in function image generation brought regression in on cluster build. If we tried to run a pipeline without image specified, it wasn't able to generate the correct image name.

ie. following `func.yaml` 
```yaml
specVersion: 0.25.0
name: test-function
namespace: default
runtime: node
registry: docker.io/zroubalik
image: ""
imageDigest: ""
build: git
...
```
fails with: 
```
🕐 Creating Pipeline resources
Error: failed to run pipeline: could not parse reference: 
```

This PR improves the error message as well.


<!-- 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
-->

- :bug: on cluster build - populate image name if not set


<!--
In addition, categorize the changes you're making using the "/kind" Prow command, example:

/kind <kind>

Supported kinds are: api-change, bug, cleanup, deprecation, removal, documentation, enhancement, performance

-->
/kind bug

<!--
*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`_*
-->



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

This pull-request has been approved by: *<a href="https://github.com/knative-sandbox/kn-plugin-func/pull/1213#" 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/1213?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox) Report
> Merging [#1213](https://codecov.io/gh/knative-sandbox/kn-plugin-func/pull/1213?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox) (937d6e0) into [main](https://codecov.io/gh/knative-sandbox/kn-plugin-func/commit/ae75e5e803fd412bef8f2c0e96d1793a77fe2c6a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox) (ae75e5e) will **increase** coverage by `0.10%`.
> The diff coverage is `36.36%`.

> :exclamation: Current head 937d6e0 differs from pull request most recent head 252325a. Consider uploading reports for the commit 252325a to get more accurate results

```diff
@@            Coverage Diff             @@
##             main    #1213      +/-   ##
==========================================
+ Coverage   45.32%   45.43%   +0.10%     
==========================================
  Files          68       68              
  Lines        9699     9709      +10     
==========================================
+ Hits         4396     4411      +15     
+ Misses       4914     4905       -9     
- Partials      389      393       +4     
```


| [Impacted Files](https://codecov.io/gh/knative-sandbox/kn-plugin-func/pull/1213?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox) | Coverage Δ | |
|---|---|---|
| [pipelines/tekton/pipeplines\_provider.go](https://codecov.io/gh/knative-sandbox/kn-plugin-func/pull/1213/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox#diff-cGlwZWxpbmVzL3Rla3Rvbi9waXBlcGxpbmVzX3Byb3ZpZGVyLmdv) | `0.00% <0.00%> (ø)` | |
| [client.go](https://codecov.io/gh/knative-sandbox/kn-plugin-func/pull/1213/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox#diff-Y2xpZW50Lmdv) | `64.67% <40.00%> (+1.67%)` | :arrow_up: |

Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox)

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

This pull-request has been approved by: *<a href="https://github.com/knative-sandbox/kn-plugin-func/pull/1213#pullrequestreview-1097713007" title="LGTM">lkingland</a>*, *<a href="https://github.com/knative-sandbox/kn-plugin-func/pull/1213#" 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)~~ [lkingland,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":[]} -->

Patch:
diff --git a/client.go b/client.go
index 55e95692..dfd4167f 100644
--- a/client.go
+++ b/client.go
@@ -728,6 +728,7 @@ func (c *Client) Deploy(ctx context.Context, path string) (err error) {
 }
 
 // RunPipeline runs a Pipeline to Build and deploy the function at path.
+// In a parameter accepts git configuration options that we don't want to persist into func.yaml.
 func (c *Client) RunPipeline(ctx context.Context, path string, git Git) (err error) {
 	go func() {
 		<-ctx.Done()
@@ -739,6 +740,29 @@ func (c *Client) RunPipeline(ctx context.Context, path string, git Git) (err err
 		err = fmt.Errorf("failed to laod function: %w", err)
 		return
 	}
+
+	// Default function registry to the client's global registry
+	if f.Registry == "" {
+		f.Registry = c.registry
+	}
+
+	// If no image name has been yet defined (not yet built/deployed), calculate.
+	// Image name is stored on the function for later use by deploy, etc.
+	// TODO: write this to .func/build instead, and populate f.Image on deploy
+	// such that local builds do not dirty the work tree.
+	if f.Image == "" {
+		if f.Image, err = f.ImageName(); err != nil {
+			return
+		}
+		//Write (save) - Serialize the function to disk
+		//Will now contain populated image name.
+		if err = f.Write(); err != nil {
+			return
+		}
+	}
+
+	// Git configuration options specified as arguments don't get saved to func.yaml,
+	// but are used in the pipeline invocation
 	f.Git = git
 
 	// Build and deploy function using Pipeline
@@ -880,7 +904,9 @@ func (c *Client) Remove(ctx context.Context, cfg Function, deleteAll bool) error
 // instance is preferred, with the remote function triggered if there is no
 // locally running instance.
 // Example:
-//  myClient.Invoke(myContext, myFunction, "local", NewInvokeMessage())
+//
+//	myClient.Invoke(myContext, myFunction, "local", NewInvokeMessage())
+//
 // The message sent to the function is defined by the invoke message.
 // See NewInvokeMessage for its defaults.
 // Functions are invoked in a manner consistent with the settings defined in
diff --git a/pipelines/tekton/pipeplines_provider.go b/pipelines/tekton/pipeplines_provider.go
index 3f454259..03293ebd 100644
--- a/pipelines/tekton/pipeplines_provider.go
+++ b/pipelines/tekton/pipeplines_provider.go
@@ -121,7 +121,7 @@ func (pp *PipelinesProvider) Run(ctx context.Context, f fn.Function) error {
 
 	registry, err := docker.GetRegistry(f.Image)
 	if err != nil {
-		return err
+		return fmt.Errorf("problem in resolving image registry name: %v", err)
 	}
 
 	pp.progressListener.Stopping()


Test patch:
diff --git a/client_test.go b/client_test.go
index f61ac95e..d9e7a244 100644
--- a/client_test.go
+++ b/client_test.go
@@ -348,8 +348,10 @@ func TestClient_New_Named(t *testing.T) {
 // Registry is the namespace at the container image registry.
 // If not prepended with the registry, it will be defaulted:
 // Examples:  "docker.io/alice"
-//            "quay.io/bob"
-//            "charlie" (becomes [DefaultRegistry]/charlie
+//
+//	"quay.io/bob"
+//	"charlie" (becomes [DefaultRegistry]/charlie
+//
 // At this time a registry namespace is required as we rely on a third-party
 // registry in all cases.  When we support in-cluster container registries,
 // this configuration parameter will become optional.
@@ -976,7 +978,7 @@ func TestClient_List_OutsideRoot(t *testing.T) {
 	}
 }
 
-// TestClient_Deploy_Image ensures that initilally the function's image
+// TestClient_Deploy_Image ensures that initially the function's image
 // member has no value (not initially deployed); the value is populated
 // upon deployment with a value derived from the function's name and currently
 // effective client registry; that the value of f.Image will take precidence
@@ -1053,6 +1055,82 @@ func TestClient_Deploy_Image(t *testing.T) {
 	}
 }
 
+// TestClient_Pipelines_Deploy_Image ensures that initially the function's image
+// member has no value (not initially deployed); the value is populated
+// upon pipeline run execution with a value derived from the function's name and currently
+// effective client registry; that the value of f.Image will take precidence
+// over .Registry, which is used to calculate a default value for image.
+func TestClient_Pipelines_Deploy_Image(t *testing.T) {
+	root, rm := Mktemp(t)
+	defer rm()
+
+	client := fn.New(
+		fn.WithPipelinesProvider(mock.NewPipelinesProvider()),
+		fn.WithRegistry("example.com/alice"))
+
+	repoUrl := "http://example-git.com/alice/myfunc.gi"
+	git := fn.Git{
+		URL: &repoUrl,
+	}
+
+	err := client.Create(fn.Function{Name: "myfunc", Runtime: "node", Root: root})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	f, err := fn.NewFunction(root)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// Upon initial creation, the value of .Image is empty
+	if f.Image != "" {
+		t.Fatalf("new function should have no image, got '%v'", f.Image)
+	}
+
+	// Upon pipeline run, the function should be populated;
+	if err = client.RunPipeline(context.Background(), root, git); err != nil {
+		t.Fatal(err)
+	}
+	f, err = fn.NewFunction(root)
+	if err != nil {
+		t.Fatal(err)
+	}
+	expected := "example.com/alice/myfunc:latest"
+	if f.Image != expected {
+		t.Fatalf("expected image '%v', got '%v'", expected, f.Image)
+	}
+	expected = "example.com/alice"
+	if f.Registry != "example.com/alice" {
+		t.Fatalf("expected registry '%v', got '%v'", expected, f.Registry)
+	}
+
+	// The value of .Image always takes precidence
+	f.Image = "registry2.example.com/bob/myfunc:latest"
+	if err = f.Write(); err != nil {
+		t.Fatal(err)
+	}
+	// Upon pipeline run, the function should be populated;
+	if err = client.RunPipeline(context.Background(), root, git); err != nil {
+		t.Fatal(err)
+	}
+	expected = "registry2.example.com/bob/myfunc:latest"
+	if f.Image != expected {
+		t.Fatalf("expected image '%v', got '%v'", expected, f.Image)
+	}
+	expected = "example.com/alice"
+	if f.Registry != "example.com/alice" {
+		// Note that according to current logic, the function's defined registry
+		// may be inaccurate.  Consider an initial deploy to registryA, followed by
+		// an explicit mutaiton of the function's .Image member.
+		// This could either remain as a documented nuance:
+		//   'The value of f.Registry is only used in the event an image name
+		//    need be derived (f.Image =="")
+		// Or we could update .Registry to always be in sync by parsing the .Image
+		t.Fatalf("expected registry '%v', got '%v'", expected, f.Registry)
+	}
+}
+
 // TestClient_Deploy_UnbuiltErrors ensures that a call to deploy a function which was not
 // fully created (ie. was only initialized, not actually built and deploys)
 // yields an expected, and informative, error.