Repository: knative/client (go)

Issue:
Add Port Name to kn service create options
<!-- If you need to report a security issue with Knative, send an email to knative-security@googlegroups.com. -->

### Feature request

Add port name to kn service create options.

For deploying a grpc server as a ksvc,  port name should be "h2c". Currently kn cli doesnt support port name.

Example of grpc server as ksvc https://knative.dev/docs/serving/samples/grpc-ping-go/

<!--
Optional Classifications, remove the '> ' to select:
/kind good-first-issue
/kind proposal
-->


Pull request:
add port name
## Description

Add provision to specify port name

## Changes

<!-- Please add list of more detailed changes. These changes should be reflected also in the commit messages -->

* Change port field from int32 to string
*  Accept port field as portname:port
* Set the container port option accordingly

## Reference

<!-- Please add the corresponding issue number which this pull request is about to fix -->
Fixes #912 

<!--
Please add an entrty to CHANGELOG.adoc file, too, as part of your Pull Request.

In the following cases, add a short description of PR to the unreleased section in CHANGELOG.adoc:

- 🎁 New feature
- 🐛 Bug fix
- ✨ Feature Update
- 🐣 Refactoring
- 🗑️ Remove feature or internal logic

See other entries in CHANGELOG.adoc as an example for how to add the entry, including a reference to the corresponding issue/PR

PLEASE DON'T ADD THAT LINE HERE IN THE PULL-REQUEST DESCRIPTION BUT DIRECTLY IN CHANGELOG.ADOC AND ADD CHANGELOG.ADOC AS PART OF YOUR PULL-REQUEST.
-->

<!--
To automatically lint go code in this pull request uncomment the line below. You get feedback as comments on your pull request then -->

<!--
/lint
-->


Patch:
diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc
index d3b9d70b..491ffc7e 100644
--- a/CHANGELOG.adoc
+++ b/CHANGELOG.adoc
@@ -18,6 +18,10 @@
 |===
 | | Description | PR
 
+| 🎁
+| Add "url" output format to return service url in service describe
+| https://github.com/knative/client/pull/916[#916]
+
 | 🐛
 | Fix panic for `kn source apiserver` and `kn source binding` describe with Sink URI
 | https://github.com/knative/client/pull/901[#901]
diff --git a/docs/cmd/kn_service_create.md b/docs/cmd/kn_service_create.md
index 4bfb25ff..2dc5aa97 100644
--- a/docs/cmd/kn_service_create.md
+++ b/docs/cmd/kn_service_create.md
@@ -27,8 +27,11 @@ kn service create NAME --image IMAGE
   # Create or replace environment variables of service 's1' using --force flag
   kn service create --force s1 --env TARGET=force --env FROM=examples --image knativesamples/helloworld
 
-  # Create a service with port 80
-  kn service create s2 --port 80 --image knativesamples/helloworld
+  # Create a service with port 8080
+  kn service create s2 --port 8080 --image knativesamples/helloworld
+
+  # Create a service with port 8080 and port name h2c
+  kn service create s2 --port h2c:8080 --image knativesamples/helloworld
 
   # Create or replace default resources of a service 's1' using --force flag
   # (earlier configured resource requests and limits will be replaced with default)
@@ -78,7 +81,7 @@ kn service create NAME --image IMAGE
       --no-cluster-local              Do not specify that the service be private. (--no-cluster-local will make the service publicly available) (default true)
       --no-lock-to-digest             Do not keep the running image for the service constant when not explicitly specifying the image. (--no-lock-to-digest pulls the image tag afresh with each new revision)
       --no-wait                       Do not wait for 'service create' operation to be completed.
-  -p, --port int32                    The port where application listens on.
+  -p, --port string                   The port where application listens on, in the format 'NAME:PORT', where 'NAME' is optional. Examples: '--port h2c:8080' , '--port 8080'.
       --pull-secret string            Image pull secret to set. An empty argument ("") clears the pull secret. The referenced secret must exist in the service's namespace.
       --request strings               The resource requirement requests for this Service. For example, 'cpu=100m,memory=256Mi'. You can use this flag multiple times. To unset a resource request, append "-" to the resource name, e.g. '--request cpu-'.
       --requests-cpu string           DEPRECATED: please use --request instead. The requested CPU (e.g., 250m).
diff --git a/docs/cmd/kn_service_describe.md b/docs/cmd/kn_service_describe.md
index 4f9d274a..41f81ecc 100644
--- a/docs/cmd/kn_service_describe.md
+++ b/docs/cmd/kn_service_describe.md
@@ -10,13 +10,27 @@ Show details of a service
 kn service describe NAME
 ```
 
+### Examples
+
+```
+
+  # Describe service 'svc' in human friendly format
+  kn service describe svc
+
+  # Describe service 'svc' in YAML format
+  kn service describe svc -o yaml
+
+  # Print only service URL
+  kn service describe svc -o url
+```
+
 ### Options
 
 ```
       --allow-missing-template-keys   If true, ignore any errors in templates when a field or map key is missing in the template. Only applies to golang and jsonpath output formats. (default true)
   -h, --help                          help for describe
   -n, --namespace string              Specify the namespace to operate in.
-  -o, --output string                 Output format. One of: json|yaml|name|go-template|go-template-file|template|templatefile|jsonpath|jsonpath-file.
+  -o, --output string                 Output format. One of: json|yaml|name|go-template|go-template-file|template|templatefile|jsonpath|jsonpath-file|url.
       --template string               Template string or path to template file to use when -o=go-template, -o=go-template-file. The template format is golang templates [http://golang.org/pkg/text/template/#pkg-overview].
   -v, --verbose                       More output.
 ```
diff --git a/docs/cmd/kn_service_update.md b/docs/cmd/kn_service_update.md
index 22763af3..87e4c80b 100644
--- a/docs/cmd/kn_service_update.md
+++ b/docs/cmd/kn_service_update.md
@@ -65,7 +65,7 @@ kn service update NAME
       --no-cluster-local              Do not specify that the service be private. (--no-cluster-local will make the service publicly available) (default true)
       --no-lock-to-digest             Do not keep the running image for the service constant when not explicitly specifying the image. (--no-lock-to-digest pulls the image tag afresh with each new revision)
       --no-wait                       Do not wait for 'service update' operation to be completed.
-  -p, --port int32                    The port where application listens on.
+  -p, --port string                   The port where application listens on, in the format 'NAME:PORT', where 'NAME' is optional. Examples: '--port h2c:8080' , '--port 8080'.
       --pull-secret string            Image pull secret to set. An empty argument ("") clears the pull secret. The referenced secret must exist in the service's namespace.
       --request strings               The resource requirement requests for this Service. For example, 'cpu=100m,memory=256Mi'. You can use this flag multiple times. To unset a resource request, append "-" to the resource name, e.g. '--request cpu-'.
       --requests-cpu string           DEPRECATED: please use --request instead. The requested CPU (e.g., 250m).
diff --git a/hack/update-deps.sh b/hack/update-deps.sh
index 8aebcf60..a69790af 100755
--- a/hack/update-deps.sh
+++ b/hack/update-deps.sh
@@ -23,7 +23,7 @@ set -o pipefail
 
 cd ${ROOT_DIR}
 
-VERSION="master"
+VERSION="release-16"
 
 # The list of dependencies that we track at HEAD and periodically
 # float forward in this repository.
diff --git a/pkg/kn/commands/service/configuration_edit_flags.go b/pkg/kn/commands/service/configuration_edit_flags.go
index a188d694..36ea73d3 100644
--- a/pkg/kn/commands/service/configuration_edit_flags.go
+++ b/pkg/kn/commands/service/configuration_edit_flags.go
@@ -51,7 +51,7 @@ type ConfigurationEditFlags struct {
 	ConcurrencyLimit           int
 	ConcurrencyUtilization     int
 	AutoscaleWindow            string
-	Port                       int32
+	Port                       string
 	Labels                     []string
 	LabelsService              []string
 	LabelsRevision             []string
@@ -207,7 +207,7 @@ func (p *ConfigurationEditFlags) addSharedFlags(command *cobra.Command) {
 		"Percentage of concurrent requests utilization before scaling up.")
 	p.markFlagMakesRevision("concurrency-utilization")
 
-	command.Flags().Int32VarP(&p.Port, "port", "p", 0, "The port where application listens on.")
+	command.Flags().StringVarP(&p.Port, "port", "p", "", "The port where application listens on, in the format 'NAME:PORT', where 'NAME' is optional. Examples: '--port h2c:8080' , '--port 8080'.")
 	p.markFlagMakesRevision("port")
 
 	command.Flags().StringArrayVarP(&p.Labels, "label", "l", []string{},
diff --git a/pkg/kn/commands/service/create.go b/pkg/kn/commands/service/create.go
index 249f3f65..0dfae39f 100644
--- a/pkg/kn/commands/service/create.go
+++ b/pkg/kn/commands/service/create.go
@@ -47,8 +47,11 @@ var create_example = `
   # Create or replace environment variables of service 's1' using --force flag
   kn service create --force s1 --env TARGET=force --env FROM=examples --image knativesamples/helloworld
 
-  # Create a service with port 80
-  kn service create s2 --port 80 --image knativesamples/helloworld
+  # Create a service with port 8080
+  kn service create s2 --port 8080 --image knativesamples/helloworld
+
+  # Create a service with port 8080 and port name h2c
+  kn service create s2 --port h2c:8080 --image knativesamples/helloworld
 
   # Create or replace default resources of a service 's1' using --force flag
   # (earlier configured resource requests and limits will be replaced with default)
diff --git a/pkg/kn/commands/service/describe.go b/pkg/kn/commands/service/describe.go
index f8191520..ea62cf36 100644
--- a/pkg/kn/commands/service/describe.go
+++ b/pkg/kn/commands/service/describe.go
@@ -20,6 +20,7 @@ import (
 	"io"
 	"sort"
 	"strconv"
+	"strings"
 
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/cli-runtime/pkg/genericclioptions"
@@ -70,6 +71,16 @@ type revisionDesc struct {
 // As this command does not do any writes/updates, it's just a matter of fallbacks.
 // [/REMOVE COMMENT WHEN MOVING TO 0.7.0]
 
+var describe_example = `
+  # Describe service 'svc' in human friendly format
+  kn service describe svc
+
+  # Describe service 'svc' in YAML format
+  kn service describe svc -o yaml
+
+  # Print only service URL
+  kn service describe svc -o url`
+
 // NewServiceDescribeCommand returns a new command for describing a service.
 func NewServiceDescribeCommand(p *commands.KnParams) *cobra.Command {
 
@@ -77,8 +88,9 @@ func NewServiceDescribeCommand(p *commands.KnParams) *cobra.Command {
 	machineReadablePrintFlags := genericclioptions.NewPrintFlags("")
 
 	command := &cobra.Command{
-		Use:   "describe NAME",
-		Short: "Show details of a service",
+		Use:     "describe NAME",
+		Short:   "Show details of a service",
+		Example: describe_example,
 		RunE: func(cmd *cobra.Command, args []string) error {
 			if len(args) != 1 {
 				return errors.New("'service describe' requires the service name given as single argument")
@@ -102,11 +114,16 @@ func NewServiceDescribeCommand(p *commands.KnParams) *cobra.Command {
 
 			// Print out machine readable output if requested
 			if machineReadablePrintFlags.OutputFlagSpecified() {
+				out := cmd.OutOrStdout()
+				if strings.ToLower(*machineReadablePrintFlags.OutputFormat) == "url" {
+					fmt.Fprintf(out, "%s\n", extractURL(service))
+					return nil
+				}
 				printer, err := machineReadablePrintFlags.ToPrinter()
 				if err != nil {
 					return err
 				}
-				return printer.PrintObj(service, cmd.OutOrStdout())
+				return printer.PrintObj(service, out)
 			}
 
 			printDetails, err = cmd.Flags().GetBool("verbose")
@@ -126,6 +143,7 @@ func NewServiceDescribeCommand(p *commands.KnParams) *cobra.Command {
 	commands.AddNamespaceFlags(flags, false)
 	flags.BoolP("verbose", "v", false, "More output.")
 	machineReadablePrintFlags.AddFlags(command)
+	command.Flag("output").Usage = fmt.Sprintf("Output format. One of: %s.", strings.Join(append(machineReadablePrintFlags.AllowedFormats(), "url"), "|"))
 	return command
 }
 
diff --git a/pkg/serving/config_changes.go b/pkg/serving/config_changes.go
index 71f05133..343610e4 100644
--- a/pkg/serving/config_changes.go
+++ b/pkg/serving/config_changes.go
@@ -40,6 +40,12 @@ type VolumeSourceType int
 const (
 	ConfigMapVolumeSourceType VolumeSourceType = iota
 	SecretVolumeSourceType
+	PortFormatErr = "The port specification '%s' is not valid. Please provide in the format 'NAME:PORT', where 'NAME' is optional. Examples: '--port h2c:8080' , '--port 8080'."
+)
+
+var (
+	UserImageAnnotationKey = "client.knative.dev/user-image"
+	ApiTooOldError         = errors.New("the service is using too old of an API format for the operation")
 )
 
 func (vt VolumeSourceType) String() string {
@@ -50,8 +56,6 @@ func (vt VolumeSourceType) String() string {
 	return names[vt]
 }
 
-var UserImageAnnotationKey = "client.knative.dev/user-image"
-
 // UpdateEnvVars gives the configuration all the env var values listed in the given map of
 // vars.  Does not touch any environment variables not mentioned, but it can add
 // new env vars and change the values of existing ones, then sort by env key name.
@@ -236,8 +240,6 @@ func UpdateRevisionTemplateAnnotation(template *servingv1.RevisionTemplateSpec,
 	return nil
 }
 
-var ApiTooOldError = errors.New("the service is using too old of an API format for the operation")
-
 // EnvToMap is an utility function to translate between the API list form of env vars, and the
 // more convenient map form.
 func EnvToMap(vars []corev1.EnvVar) (map[string]string, error) {
@@ -331,14 +333,34 @@ func UpdateContainerArg(template *servingv1.RevisionTemplateSpec, arg []string)
 	return nil
 }
 
-// UpdateContainerPort updates container with a give port
-func UpdateContainerPort(template *servingv1.RevisionTemplateSpec, port int32) error {
+// UpdateContainerPort updates container with a given name:port
+func UpdateContainerPort(template *servingv1.RevisionTemplateSpec, port string) error {
 	container, err := ContainerOfRevisionTemplate(template)
 	if err != nil {
 		return err
 	}
+
+	var containerPort int64
+	var name string
+
+	elements := strings.SplitN(port, ":", 2)
+	if len(elements) == 2 {
+		name = elements[0]
+		containerPort, err = strconv.ParseInt(elements[1], 10, 32)
+		if err != nil {
+			return fmt.Errorf(PortFormatErr, port)
+		}
+	} else {
+		name = ""
+		containerPort, err = strconv.ParseInt(elements[0], 10, 32)
+		if err != nil {
+			return fmt.Errorf(PortFormatErr, port)
+		}
+	}
+
 	container.Ports = []corev1.ContainerPort{{
-		ContainerPort: port,
+		ContainerPort: int32(containerPort),
+		Name:          name,
 	}}
 	return nil
 }


Test patch:
diff --git a/lib/test/broker.go b/lib/test/broker.go
index 99dcb953..3784f176 100644
--- a/lib/test/broker.go
+++ b/lib/test/broker.go
@@ -19,14 +19,16 @@ import (
 	"time"
 
 	"k8s.io/apimachinery/pkg/util/wait"
+	"knative.dev/eventing/pkg/apis/eventing/v1beta1"
 )
 
 // LabelNamespaceForDefaultBroker adds label 'knative-eventing-injection=enabled' to the configured namespace
 func LabelNamespaceForDefaultBroker(r *KnRunResultCollector) error {
-	_, err := Kubectl{}.Run("label", "namespace", r.KnTest().Kn().Namespace(), "knative-eventing-injection=enabled")
+	cmd := []string{"label", "namespace", r.KnTest().Kn().Namespace(), v1beta1.InjectionAnnotation + "=enabled"}
+	_, err := Kubectl{}.Run(cmd...)
 
 	if err != nil {
-		r.T().Fatalf("Error executing 'kubectl label namespace %s knative-eventing-injection=enabled'. Error: %s", r.KnTest().Kn().Namespace(), err.Error())
+		r.T().Fatalf("error executing '%s': %s", strings.Join(cmd, " "), err.Error())
 	}
 
 	return wait.PollImmediate(10*time.Second, 5*time.Minute, func() (bool, error) {
@@ -41,8 +43,9 @@ func LabelNamespaceForDefaultBroker(r *KnRunResultCollector) error {
 
 // UnlabelNamespaceForDefaultBroker removes label 'knative-eventing-injection=enabled' from the configured namespace
 func UnlabelNamespaceForDefaultBroker(r *KnRunResultCollector) {
-	_, err := Kubectl{}.Run("label", "namespace", r.KnTest().Kn().Namespace(), "knative-eventing-injection-")
+	cmd := []string{"label", "namespace", r.KnTest().Kn().Namespace(), v1beta1.InjectionAnnotation + "-"}
+	_, err := Kubectl{}.Run(cmd...)
 	if err != nil {
-		r.T().Fatalf("Error executing 'kubectl label namespace %s knative-eventing-injection-'. Error: %s", r.KnTest().Kn().Namespace(), err.Error())
+		r.T().Fatalf("error executing '%s': %s", strings.Join(cmd, " "), err.Error())
 	}
 }
diff --git a/pkg/kn/commands/service/create_test.go b/pkg/kn/commands/service/create_test.go
index d31ec821..f056267c 100644
--- a/pkg/kn/commands/service/create_test.go
+++ b/pkg/kn/commands/service/create_test.go
@@ -540,7 +540,7 @@ func TestServiceCreateMaxMinScale(t *testing.T) {
 	}
 
 	if *template.Spec.ContainerConcurrency != int64(100) {
-		t.Fatalf("container concurrency not set to given value 1000")
+		t.Fatalf("container concurrency not set to given value 100")
 	}
 }
 
diff --git a/pkg/kn/commands/service/describe_test.go b/pkg/kn/commands/service/describe_test.go
index e86d36bd..381365ab 100644
--- a/pkg/kn/commands/service/describe_test.go
+++ b/pkg/kn/commands/service/describe_test.go
@@ -506,6 +506,23 @@ func TestServiceDescribeMachineReadable(t *testing.T) {
 	r.Validate()
 }
 
+func TestServiceDescribeURL(t *testing.T) {
+	client := knclient.NewMockKnServiceClient(t)
+
+	// Recording:
+	r := client.Recorder()
+
+	// Prepare service
+	expectedService := createTestService("foo", []string{"rev1", "rev2"}, goodConditions())
+	r.GetService("foo", &expectedService, nil)
+
+	output, err := executeServiceCommand(client, "describe", "foo", "-o", "url")
+	assert.NilError(t, err)
+	assert.Assert(t, util.ContainsAll(output, "foo.default.example.com"))
+
+	r.Validate()
+}
+
 func validateServiceOutput(t *testing.T, service string, output string) {
 	assert.Assert(t, cmp.Regexp("Name:\\s+"+service, output))
 	assert.Assert(t, cmp.Regexp("Namespace:\\s+default", output))
diff --git a/pkg/kn/commands/service/update_test.go b/pkg/kn/commands/service/update_test.go
index 6b7ee027..832fb0ee 100644
--- a/pkg/kn/commands/service/update_test.go
+++ b/pkg/kn/commands/service/update_test.go
@@ -350,7 +350,7 @@ func TestServiceUpdateMaxMinScale(t *testing.T) {
 	}
 
 	if *template.Spec.ContainerConcurrency != int64(100) {
-		t.Fatalf("container concurrency not set to given value 1000")
+		t.Fatalf("container concurrency not set to given value 100")
 	}
 
 }
diff --git a/pkg/serving/config_changes_test.go b/pkg/serving/config_changes_test.go
index 420faa26..5bb0904a 100644
--- a/pkg/serving/config_changes_test.go
+++ b/pkg/serving/config_changes_test.go
@@ -15,6 +15,7 @@
 package serving
 
 import (
+	"fmt"
 	"reflect"
 	"strconv"
 	"testing"
@@ -285,21 +286,52 @@ func TestUpdateContainerArg(t *testing.T) {
 
 func TestUpdateContainerPort(t *testing.T) {
 	template, _ := getRevisionTemplate()
-	err := UpdateContainerPort(template, 8888)
-	assert.NilError(t, err)
-	// Verify update is successful or not
-	checkPortUpdate(t, template, 8888)
-	// update template with container port info
-	template.Spec.Containers[0].Ports[0].ContainerPort = 9090
-	err = UpdateContainerPort(template, 80)
-	assert.NilError(t, err)
-	// Verify that given port overrides the existing container port
-	checkPortUpdate(t, template, 80)
-}
-
-func checkPortUpdate(t *testing.T, template *servingv1.RevisionTemplateSpec, port int32) {
-	if template.Spec.Containers[0].Ports[0].ContainerPort != port {
-		t.Error("Failed to update the container port")
+	for _, tc := range []struct {
+		name    string
+		input   string
+		isErr   bool
+		expPort int32
+		expName string
+	}{{
+		name:    "only port 8888",
+		input:   "8888",
+		expPort: int32(8888),
+	}, {
+		name:    "name and port h2c:8080",
+		input:   "h2c:8080",
+		expPort: int32(8080),
+		expName: "h2c",
+	}, {
+		name:  "error case - not correct format",
+		input: "h2c:800000000000000000",
+		isErr: true,
+	}, {
+		name:  "error case - empty port",
+		input: "h2c:",
+		isErr: true,
+	}, {
+		name:  "error case - wrong format",
+		input: "8080:h2c",
+		isErr: true,
+	}, {
+		name:  "error case - multiple :",
+		input: "h2c:8080:proto",
+		isErr: true,
+	}, {
+		name:    "empty name no error",
+		input:   ":8888",
+		expPort: int32(8888),
+	}} {
+		t.Run(tc.name, func(t *testing.T) {
+			err := UpdateContainerPort(template, tc.input)
+			if tc.isErr {
+				assert.Error(t, err, fmt.Sprintf(PortFormatErr, tc.input))
+			} else {
+				assert.NilError(t, err)
+				assert.Equal(t, template.Spec.Containers[0].Ports[0].ContainerPort, tc.expPort)
+				assert.Equal(t, template.Spec.Containers[0].Ports[0].Name, tc.expName)
+			}
+		})
 	}
 }
 
diff --git a/test/common.sh b/test/common.sh
index b0c8f4d3..02ba4e77 100644
--- a/test/common.sh
+++ b/test/common.sh
@@ -36,6 +36,11 @@ function knative_setup() {
 
   if [ "${eventing_version}" = "latest" ]; then
     start_latest_knative_eventing
+
+    # install the sugar controller
+    kubectl apply --filename https://storage.googleapis.com/knative-nightly/eventing/latest/eventing-sugar-controller.yaml
+    wait_until_pods_running knative-eventing || return 1
+
   else
     start_release_knative_eventing "${eventing_version}"
   fi