feat: deploy success message also displays the namespace #1090

vyasgun · 2022-06-29T18:15:38Z

Changes

Added the function's namespace to the success message after deploy
If the namespace in function obj is blank, display current context namespace

Release Note

Prints the namespace when a function is deployed

knative-prow · 2022-06-29T18:15:43Z

@vyasgun: The label(s) kind/<kind> cannot be applied, because the repository doesn't have them.

In response to this:

Changes

Added the function's namespace to the success message after deploy

If the namespace in function obj is blank, display default

/kind

Fixes #1083

Release Note
Docs

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

codecov · 2022-06-29T18:19:38Z

Codecov Report

Merging #1090 (f16b3e9) into main (8109186) will decrease coverage by 2.18%.
The diff coverage is 23.07%.

@@            Coverage Diff             @@
##             main    #1090      +/-   ##
==========================================
- Coverage   47.22%   45.03%   -2.19%     
==========================================
  Files          59       64       +5     
  Lines        8028     8456     +428     
==========================================
+ Hits         3791     3808      +17     
- Misses       3855     4268     +413     
+ Partials      382      380       -2

Impacted Files	Coverage Δ
knative/deployer.go	`6.12% <0.00%> (-0.04%)`	⬇️
pipelines/tekton/pipeplines_provider.go	`0.00% <0.00%> (ø)`
client.go	`61.55% <100.00%> (+1.07%)`	⬆️
pipelines/tekton/tasks.go	`0.00% <0.00%> (ø)`
pipelines/tekton/validate.go	`84.61% <0.00%> (ø)`
pipelines/tekton/client.go	`0.00% <0.00%> (ø)`
pipelines/tekton/resources.go	`0.00% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8109186...f16b3e9. Read the comment docs.

zroubalik · 2022-06-29T20:17:50Z

client.go

@@ -705,10 +706,21 @@ func (c *Client) Deploy(ctx context.Context, path string) (err error) {
 	// Deploy a new or Update the previously-deployed Function
 	c.progressListener.Increment("Deploying function to the cluster")
 	result, err := c.deployer.Deploy(ctx, f)
+
+	var namespace string


The namespace is already resolved in c.deployer.Deploy(), might be better to set it in the fn.DeploymentResult and reuse here.

Just to clarify, should I add a field for namespace in the DeploymentResult type and populate it when it's resolved in c.deployer.Deploy()?

Made the above change in the latest commit.

zroubalik · 2022-06-29T20:18:40Z

client.go

 	if result.Status == Deployed {
-		c.progressListener.Increment(fmt.Sprintf("Function deployed at URL: %v", result.URL))
+		c.progressListener.Increment(fmt.Sprintf("Function deployed in namespace %q and exposed at URL: %v", namespace, result.URL))


Suggested change

c.progressListener.Increment(fmt.Sprintf("Function deployed in namespace %q and exposed at URL: %v", namespace, result.URL))

c.progressListener.Increment(fmt.Sprintf("Function deployed in namespace %q and exposed at URL: \n%v", namespace, result.URL))

See the comment: #1083 (comment)

zroubalik · 2022-06-29T20:19:38Z

client.go

 	} else if result.Status == Updated {
-		c.progressListener.Increment(fmt.Sprintf("Function updated at URL: %v", result.URL))
+		c.progressListener.Increment(fmt.Sprintf("Function updated in namespace %q and exposed at URL: %v", namespace, result.URL))


Suggested change

c.progressListener.Increment(fmt.Sprintf("Function updated in namespace %q and exposed at URL: %v", namespace, result.URL))

c.progressListener.Increment(fmt.Sprintf("Function updated in namespace %q and exposed at URL: \n%v", namespace, result.URL))

See the comment: #1083 (comment)

zroubalik

Please update On cluster build related messages as well:
https://github.com/knative-sandbox/kn-plugin-func/blob/81091863e7e02681b5951b211bbba69282d0a430/pipelines/tekton/pipeplines_provider.go#L157-L159

lkingland

/lgtm !

This is nice information to have very readily available on deploy.

/hold for others to take a look

zroubalik

/lgtm thanks for the contribution!

knative-prow · 2022-07-01T10:12:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lkingland, vyasgun, zroubalik

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~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

vyasgun · 2022-07-01T10:16:58Z

@zroubalik @lkingland Thanks for the reviews. I am stuck on the codecov part. I have added changes to the Deploy function but I can't see a way to use mock clients for writing unit tests.

zroubalik · 2022-07-01T19:34:27Z

@vyasgun I think that's okay, the test that you have added is imho enough for this feature

lance

/unhold

knative-prow bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jun 29, 2022

vyasgun force-pushed the pr/cli-namespace branch from a5ceac9 to e3ed504 Compare June 29, 2022 18:21

lance changed the title ~~Changes so func deploy success message also displays the namespace~~ feat: deploy success message also displays the namespace Jun 29, 2022

vyasgun added 2 commits June 30, 2022 01:18

func deploy should display namespace

a88d820

Modified e2e to consider namespace

8eed50a

vyasgun force-pushed the pr/cli-namespace branch from 980e91d to 8eed50a Compare June 29, 2022 19:48

zroubalik reviewed Jun 29, 2022

View reviewed changes

Added Namespace field in DeploymentResult struct

38db768

vyasgun force-pushed the pr/cli-namespace branch from 3da3be4 to 38db768 Compare June 30, 2022 10:20

knative-prow bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 30, 2022

Modified unit test for client deploy

1eaca79

vyasgun force-pushed the pr/cli-namespace branch from 727579b to 1eaca79 Compare June 30, 2022 11:24

e2e fix

f16b3e9

vyasgun force-pushed the pr/cli-namespace branch from b7b8b91 to f16b3e9 Compare June 30, 2022 13:47

lkingland approved these changes Jul 1, 2022

View reviewed changes

knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 1, 2022

knative-prow bot assigned lkingland Jul 1, 2022

knative-prow bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 1, 2022

zroubalik approved these changes Jul 1, 2022

View reviewed changes

knative-prow bot assigned zroubalik Jul 1, 2022

lance reviewed Jul 6, 2022

View reviewed changes

knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 6, 2022

knative-prow bot merged commit afd224f into knative:main Jul 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: deploy success message also displays the namespace #1090

feat: deploy success message also displays the namespace #1090

vyasgun commented Jun 29, 2022 •

edited

knative-prow bot commented Jun 29, 2022

Changes

codecov bot commented Jun 29, 2022 •

edited

zroubalik Jun 29, 2022

vyasgun Jun 30, 2022 •

edited

zroubalik Jun 29, 2022

zroubalik Jun 29, 2022

zroubalik left a comment

lkingland left a comment

zroubalik left a comment

knative-prow bot commented Jul 1, 2022

vyasgun commented Jul 1, 2022

zroubalik commented Jul 1, 2022

lance left a comment

	c.progressListener.Increment(fmt.Sprintf("Function deployed in namespace %q and exposed at URL: %v", namespace, result.URL))
	c.progressListener.Increment(fmt.Sprintf("Function deployed in namespace %q and exposed at URL: \n%v", namespace, result.URL))

feat: deploy success message also displays the namespace #1090

feat: deploy success message also displays the namespace #1090

Conversation

vyasgun commented Jun 29, 2022 • edited

Changes

knative-prow bot commented Jun 29, 2022

Changes

codecov bot commented Jun 29, 2022 • edited

Codecov Report

zroubalik Jun 29, 2022

Choose a reason for hiding this comment

vyasgun Jun 30, 2022 • edited

Choose a reason for hiding this comment

zroubalik Jun 29, 2022

Choose a reason for hiding this comment

zroubalik Jun 29, 2022

Choose a reason for hiding this comment

zroubalik left a comment

Choose a reason for hiding this comment

lkingland left a comment

Choose a reason for hiding this comment

zroubalik left a comment

Choose a reason for hiding this comment

knative-prow bot commented Jul 1, 2022

vyasgun commented Jul 1, 2022

zroubalik commented Jul 1, 2022

lance left a comment

Choose a reason for hiding this comment

vyasgun commented Jun 29, 2022 •

edited

codecov bot commented Jun 29, 2022 •

edited

vyasgun Jun 30, 2022 •

edited