[kn-admin]Added utils to update configmap #40

zhanggbj · 2020-06-04T08:48:00Z

Added utils to update configmp, as this is very common for kn-admin operations
Chores to refine vars or logs

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

zhanggbj · 2020-06-04T09:16:36Z

@rhuss @maximilien
The integration test failed due to incompatible packages, looks like it is not one of kn-admin, woudl you please help to take a look?

# k8s.io/client-go/rest
../../../pkg/mod/k8s.io/[email protected]+incompatible/rest/request.go:598:31: not enough arguments in call to watch.NewStreamWatcher
	have (*versioned.Decoder)
	want (watch.Decoder, watch.Reporter)
ERROR: cluster setup failed

rhuss · 2020-06-04T10:03:44Z

Yep, I will give a look later today (have some meetings soon)

maximilien

Missing unit tests and integration tests. Thanks.

maximilien · 2020-06-04T22:54:23Z

plugins/admin/README.adoc

@@ -120,15 +120,15 @@ Use "kn admin autoscaling [command] --help" for more information about a comman
 ====
 ----
 $ kn admin domain set --custom-domain mydomain.com
-Updated Knative route domain mydomain.com
+Set Knative route domain mydomain.com


Change to “... route to domain ...”

Same question as below.

maximilien · 2020-06-04T22:54:46Z

plugins/admin/README.adoc

 ----
 ====

 .Update a custom domain with --selector and Service with a label app=v1 will use test.com
 ====
 ----
 $ kn admin domain set --custom-domain test.com --selector app=v1
-Updated Knative route domain test.com
+Set Knative route domain test.com with selector [app=v1]


Same as above add “to”

@maximilien
As Knative supports multiple domains, I'm wondering if set route domain to is more like Knative only support one domain?

IMO if use "set" it implies setting the single value. Otherwise, I would expect an "Add route".

How would one manage multiple routes here ? If this is only about a 'default' route maybe "Set default Knative route domain ..." is better choice.

Also, I wonder whether the "Knative" is not redundant and maybe we should talk about a "service" instead of a "route" here, as route is an internal concept mostly hidden in kn. We are always talking about a service's or revsion's URL.

@rhuss
In fact, I think in this context it is more about domain other than route.

Knative support to have multiple domains as below, in kn-admin we want to help with add and remove domains. I'm thinking maybe kn admin domain add is more proper here and perhaps also change route domain to domain as it may be a little bit confusing with the Knative Route resource.

@rhuss @maximilien what do you think? Thanks!

data: # example.org will be used for routes having app=prod. example.org: | selector: app: prod # Default value for domain, for routes that does not have app=prod labels. # Although it will match all routes, it is the least-specific rule so it # will only be used if no other domain matches. example.com: ""

Yeah, the "route" confused me a bit. Maybe just referring to the domain without any revision/service prefix makes more sense.

For this example, "Add" makes more sense as it also emphasises that you add a domain with the selector.

For general wording of the command itself (set/unset), it's a bit tricky.
As you can have multiple things that can be identified, having "create/delete" is typically a better wording for commands than "set/unset" as this more implies a single entity to manage. This is how we do it for the Knative entities like service or sources.

However, as you also can override with "set" then "add" would be misleading. Not sure if an "create/update/delete" makes sense, as this would add a possibly too fine-grained workflow (and would require the user to know whether a domain already is adder).

I hope we can implement the knative story that introduces an "apply" verb soon, which would be useful here, too: "apply" essentially mean, create-or-update and is idempotent (you can call it with the same argument many times, but only get a single result). This is the case for you command here.

So, if this were a core kn command, I probably would suggest a

# Add or update the given domain with the given selector kn admin domain apply .... # Remove the domain kn admin domain remove ....

as this aligns with the other verbs used in kn.

@rhuss I feel the same way. kn-admin itself should be consistent, now we have add/update/etc. This need refine and perhaps this change can wait for and follow kn client story you mentioned.
How about I open another chore item to follow it (#53) and we can complete this PR first.

IMO, set/unset suit better for properties like domain,
while create/update/apply/deploy operates on the actual resources.

@navidshaikh thanks and we can continue subcommand wording in #53

plugins/admin/pkg/command/autoscaling/update.go

plugins/admin/pkg/command/domain/set.go

plugins/admin/pkg/command/registry/add.go

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

zhanggbj · 2020-06-07T16:16:59Z

@maximilien
I addressed your comments, and would you please help to take a look? Thanks!

e2e test case not ready yet, covered by [kn-admin] Add Unit Test and e2e test #14
Kpow integration tests failures due to repo settings (Roland is taking care)

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

rhuss

Change looks good to, thanks !

Only a small question about the wording and also I'm not clear how multiple domains can be managed (e.g also be deleted)

zhanggbj · 2020-06-10T06:06:42Z

@rhuss
Thanks! As the example here, kn-admin will also add the domains with/without the selector. When users need to remove a domain, they only need to provide the domain name.
And kn admin domain list is working in progress, to list all the added domains and with selectors if any.

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

- Added utils to update configmp - Chores to refine vars or logs

mattmoor · 2020-06-21T14:55:57Z

plugins/admin/vendor/k8s.io/apimachinery/pkg/util/wait/doc.go

@@ -0,0 +1,19 @@
+/*
+Copyright 2014 The Kubernetes Authors.


Incorrect copyright year:

Suggested change

Copyright 2014 The Kubernetes Authors.

Copyright 2020 The Kubernetes Authors.

mattmoor · 2020-06-21T14:55:58Z

plugins/admin/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go

@@ -0,0 +1,512 @@
+/*
+Copyright 2014 The Kubernetes Authors.


Incorrect copyright year:

Suggested change

Copyright 2014 The Kubernetes Authors.

Copyright 2020 The Kubernetes Authors.

mattmoor · 2020-06-21T14:55:58Z

plugins/admin/vendor/k8s.io/client-go/util/retry/util.go

@@ -0,0 +1,105 @@
+/*
+Copyright 2016 The Kubernetes Authors.


Incorrect copyright year:

Suggested change

Copyright 2016 The Kubernetes Authors.

Copyright 2020 The Kubernetes Authors.

zhanggbj · 2020-06-21T14:56:16Z

@rhuss As discussed in #40 (comment), I opened another chore item to follow command wording create/apply/update (#53).

@rhuss @maximilien Please help to review again and hope we can merge this first before the vendor pkg change. thanks!

rhuss

looks good for me, let's check in a follow PR about the concrete wording.

/lgtm

rhuss · 2020-06-23T14:52:39Z

/hold

rhuss · 2020-06-23T18:27:25Z

@maximilien please unhold if this looks ok for you, so that it gets merged.

maximilien

LGTM let me ask @navidshaikh if he has not seen this before.

I also don’t see tests for registry/add. Please address and let me know and I can do final pass.

knative-prow-robot · 2020-06-23T20:37:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maximilien, rhuss, zhanggbj

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:

~~plugins/admin/OWNERS~~ [maximilien,rhuss,zhanggbj]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

zhanggbj · 2020-06-24T02:28:01Z

@maximilien Thanks Max. For test, we have UT for each command and also for registry/add
e2e test is still working in progress by #14

navidshaikh

suggested a few nits.

Unrelated to the changes in PR, noticed error returned by splitByEqualSign is not handled.
If incorrect selector is given, the error is reported from configmap update API call instead of plugin flag handling (can be fixed in a separate PR).

client-contrib/plugins/admin/pkg/command/domain/set.go

Line 62 in 5213e9a

k, v, _ := splitByEqualSign(label)

navidshaikh · 2020-06-24T05:14:38Z

plugins/admin/pkg/command/autoscaling/update.go

+  kn admin autoscaling update --scale-to-zero
+  # To disable scale-to-zero


Suggested change

kn admin autoscaling update --scale-to-zero

# To disable scale-to-zero

kn admin autoscaling update --scale-to-zero

# To disable scale-to-zero

@navidshaikh yes, I agree, this is a known issue and will be handled #41

navidshaikh · 2020-06-24T05:19:39Z

plugins/admin/pkg/command/domain/domain.go

-kn admin domain set - to set Knative route domain
-
-kn admin domain unset - to unset Knative route domain`,
+		Long:  `Manage default route domain or custom route domain for Service with selectors.`,


Suggested change

Long: `Manage default route domain or custom route domain for Service with selectors.`,

Long: `Manage default route domain or custom route domain for service(s) with selectors`,

navidshaikh · 2020-06-24T05:27:08Z

plugins/admin/pkg/command/domain/set.go

+  # To set a default route domain
+  kn admin domain set --custom-domain mydomain.com
+
+  # To set a route domain for service having label app=v1


Suggested change

# To set a route domain for service having label app=v1

# To set a route domain for service(s) having label 'app=v1'

@navidshaikh Thanks for the suggestion. Will update in another PR to refine #41

navidshaikh · 2020-06-24T05:45:49Z

plugins/admin/pkg/command/domain/unset.go

+	"knative.dev/client-contrib/plugins/admin/pkg/command/utils"
+
 	"knative.dev/client-contrib/plugins/admin/pkg"


Suggested change

"knative.dev/client-contrib/plugins/admin/pkg/command/utils"

"knative.dev/client-contrib/plugins/admin/pkg"

"knative.dev/client-contrib/plugins/admin/pkg/command/utils"

"knative.dev/client-contrib/plugins/admin/pkg"

navidshaikh · 2020-06-24T05:46:36Z

plugins/admin/pkg/command/autoscaling/autoscaling.go

-		Long: `Manage autoscaling provided by Knative Pod Autoscaler (KPA). For example:
-
-kn admin autoscaling update - to manage autoscaling config`,
+		Long:  `Manage autoscaling provided by Knative Pod Autoscaler (KPA).`,


Suggested change

Long: `Manage autoscaling provided by Knative Pod Autoscaler (KPA).`,

Long: `Manage autoscaling provided by Knative Pod Autoscaler (KPA)`,

navidshaikh · 2020-06-24T05:55:45Z

plugins/admin/pkg/command/registry/add.go

-  --username=[REGISTRY_USER] \
-  --password=[REGISTRY_PASSWORD]`,
+		Short: "Add registry with credentials",
+		Long:  `Add registry with credentials to enable Service deployment from it`,


Suggested change

Long: `Add registry with credentials to enable Service deployment from it`,

Long: `Add registry with credentials to enable service deployment from it`,

navidshaikh · 2020-06-24T05:56:06Z

plugins/admin/pkg/command/registry/registry.go

-  --email=[REGISTRY_EMAIL] \
-  --username=[REGISTRY_USER] \
-  --password=[REGISTRY_PASSWORD]`,
+		Long:  `Manage Service deployment from a registry with credentials`,


Suggested change

Long: `Manage Service deployment from a registry with credentials`,

Long: `Manage service deployment from a registry with credentials`,

navidshaikh · 2020-06-24T05:56:24Z

plugins/admin/pkg/command/utils/utils.go

+	"k8s.io/client-go/util/retry"
+)
+
+func UpdateConfigMap(client kubernetes.Interface, desiredCm *corev1.ConfigMap) error {


navidshaikh · 2020-06-24T06:08:46Z

plugins/admin/README.adoc

 ----
 ====

 .Update a custom domain with --selector and Service with a label app=v1 will use test.com
 ====
 ----
 $ kn admin domain set --custom-domain test.com --selector app=v1
-Updated Knative route domain test.com
+Set Knative route domain test.com with selector [app=v1]


IMO, set/unset suit better for properties like domain,
while create/update/apply/deploy operates on the actual resources.

knative-prow-robot

@navidshaikh: 6 warnings.

In response to this:

/lint

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.

knative-prow-robot · 2020-06-24T06:21:37Z

plugins/admin/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go

+	"k8s.io/apimachinery/pkg/util/runtime"
+)
+
+// For any test of the style:


Golint comments: comment on exported var ForeverTestTimeout should be of the form "ForeverTestTimeout ...". More info.

knative-prow-robot · 2020-06-24T06:21:37Z

plugins/admin/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go

+	wg sync.WaitGroup
+}
+
+func (g *Group) Wait() {


Golint comments: exported method Group.Wait should have comment or be unexported. More info.

knative-prow-robot · 2020-06-24T06:21:37Z

plugins/admin/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go

+
+// WaitFunc creates a channel that receives an item every time a test
+// should be executed and is closed when the last test should be invoked.
+type WaitFunc func(done <-chan struct{}) <-chan struct{}


Suggested change

type WaitFunc func(done <-chan struct{}) <-chan struct{}

type Func func(done <-chan struct{}) <-chan struct{}

Golint naming: type name will be used as wait.WaitFunc by other packages, and that stutters; consider calling this Func. More info.

knative-prow-robot · 2020-06-24T06:21:38Z

plugins/admin/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go

+// When the done channel is closed, because the golang `select` statement is
+// "uniform pseudo-random", the `fn` might still run one or multiple time,
+// though eventually `WaitFor` will return.
+func WaitFor(wait WaitFunc, fn ConditionFunc, done <-chan struct{}) error {


Suggested change

func WaitFor(wait WaitFunc, fn ConditionFunc, done <-chan struct{}) error {

func For(wait WaitFunc, fn ConditionFunc, done <-chan struct{}) error {

Golint naming: func name will be used as wait.WaitFor by other packages, and that stutters; consider calling this For. More info.

knative-prow-robot · 2020-06-24T06:21:38Z

plugins/admin/vendor/k8s.io/client-go/util/retry/util.go

+//     ...
+//
+// TODO: Make Backoff an interface?
+func RetryOnConflict(backoff wait.Backoff, fn func() error) error {


Suggested change

func RetryOnConflict(backoff wait.Backoff, fn func() error) error {

func OnConflict(backoff wait.Backoff, fn func() error) error {

Golint naming: func name will be used as retry.RetryOnConflict by other packages, and that stutters; consider calling this OnConflict. More info.

knative-prow-robot · 2020-06-24T06:21:38Z

plugins/admin/pkg/command/utils/utils.go

+	"k8s.io/client-go/util/retry"
+)
+
+func UpdateConfigMap(client kubernetes.Interface, desiredCm *corev1.ConfigMap) error {


Golint comments: exported function UpdateConfigMap should have comment or be unexported. More info.

maximilien · 2020-06-24T19:54:51Z

@maximilien Thanks Max. For test, we have UT for each command and also for registry/add
e2e test is still working in progress by #14

OK cool. Let me know when you address the last comments from @navidshaikh and we can merge this. Thx.

rhuss · 2020-06-25T06:25:07Z

/unhold

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jun 4, 2020

knative-prow-robot requested review from maximilien and navidshaikh June 4, 2020 08:48

knative-prow-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 4, 2020

mattmoor reviewed Jun 4, 2020

View reviewed changes

zhanggbj changed the title ~~Added utils to update configmap~~ [kn-admin]Added utils to update configmap Jun 4, 2020

mattmoor reviewed Jun 4, 2020

View reviewed changes

zhanggbj force-pushed the chore branch from 244d944 to 9987283 Compare June 4, 2020 08:52

mattmoor reviewed Jun 4, 2020

View reviewed changes

maximilien suggested changes Jun 4, 2020

View reviewed changes

zhanggbj force-pushed the chore branch from 9987283 to b868bf5 Compare June 7, 2020 16:14

mattmoor reviewed Jun 7, 2020

View reviewed changes

zhanggbj force-pushed the chore branch from b868bf5 to f70dbde Compare June 7, 2020 16:20

mattmoor reviewed Jun 7, 2020

View reviewed changes

zhanggbj force-pushed the chore branch from f70dbde to 7cd9c18 Compare June 10, 2020 02:22

mattmoor reviewed Jun 10, 2020

View reviewed changes

rhuss reviewed Jun 10, 2020

View reviewed changes

This was referenced Jun 11, 2020

[kn-admin]Add support to remove registry with credentials #47

Merged

[kn-admin] Improve the command examples in usage #43

Closed

zhanggbj force-pushed the chore branch from 7cd9c18 to 2f2cda2 Compare June 11, 2020 12:49

mattmoor reviewed Jun 11, 2020

View reviewed changes

zhanggbj mentioned this pull request Jun 19, 2020

[kn-admin] Refine subcommand wording #53

Open

Added utils to update configmap

9b5e793

- Added utils to update configmp - Chores to refine vars or logs

zhanggbj force-pushed the chore branch from 2f2cda2 to 9b5e793 Compare June 21, 2020 14:55

mattmoor reviewed Jun 21, 2020

View reviewed changes

zhanggbj requested review from maximilien and rhuss June 22, 2020 13:59

rhuss approved these changes Jun 23, 2020

View reviewed changes

knative-prow-robot assigned rhuss Jun 23, 2020

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 23, 2020

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 23, 2020

maximilien approved these changes Jun 23, 2020

View reviewed changes

navidshaikh reviewed Jun 24, 2020

View reviewed changes

knative-prow-robot reviewed Jun 24, 2020

View reviewed changes

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 25, 2020

knative-prow-robot merged commit 969cebc into knative:master Jun 25, 2020

zhanggbj mentioned this pull request Jun 25, 2020

[kn-admin] Improve domain set command #41

Closed

chaozbj mentioned this pull request Jun 29, 2020

Improve domain subcommands and some wording suggestions #62

Merged

	Copyright 2014 The Kubernetes Authors.
	Copyright 2020 The Kubernetes Authors.

	Copyright 2016 The Kubernetes Authors.
	Copyright 2020 The Kubernetes Authors.

		kn admin autoscaling update --scale-to-zero
		# To disable scale-to-zero

	Long: `Manage default route domain or custom route domain for Service with selectors.`,
	Long: `Manage default route domain or custom route domain for service(s) with selectors`,

	# To set a route domain for service having label app=v1
	# To set a route domain for service(s) having label 'app=v1'

		"knative.dev/client-contrib/plugins/admin/pkg/command/utils"

		"knative.dev/client-contrib/plugins/admin/pkg"

	Long: `Manage autoscaling provided by Knative Pod Autoscaler (KPA).`,
	Long: `Manage autoscaling provided by Knative Pod Autoscaler (KPA)`,

	Long: `Add registry with credentials to enable Service deployment from it`,
	Long: `Add registry with credentials to enable service deployment from it`,

	Long: `Manage Service deployment from a registry with credentials`,
	Long: `Manage service deployment from a registry with credentials`,

	type WaitFunc func(done <-chan struct{}) <-chan struct{}
	type Func func(done <-chan struct{}) <-chan struct{}

	func WaitFor(wait WaitFunc, fn ConditionFunc, done <-chan struct{}) error {
	func For(wait WaitFunc, fn ConditionFunc, done <-chan struct{}) error {

	func RetryOnConflict(backoff wait.Backoff, fn func() error) error {
	func OnConflict(backoff wait.Backoff, fn func() error) error {

[kn-admin]Added utils to update configmap #40

[kn-admin]Added utils to update configmap #40

Conversation

zhanggbj commented Jun 4, 2020

mattmoor left a comment

Choose a reason for hiding this comment

zhanggbj commented Jun 4, 2020

rhuss commented Jun 4, 2020

maximilien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhanggbj Jun 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattmoor left a comment

Choose a reason for hiding this comment

zhanggbj commented Jun 7, 2020

mattmoor left a comment

Choose a reason for hiding this comment

mattmoor left a comment

Choose a reason for hiding this comment

rhuss left a comment

Choose a reason for hiding this comment

zhanggbj commented Jun 10, 2020

mattmoor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhanggbj commented Jun 21, 2020 • edited Loading

rhuss left a comment

Choose a reason for hiding this comment

rhuss commented Jun 23, 2020

rhuss commented Jun 23, 2020

maximilien left a comment • edited Loading

Choose a reason for hiding this comment

knative-prow-robot commented Jun 23, 2020

zhanggbj commented Jun 24, 2020

navidshaikh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

navidshaikh Jun 24, 2020 • edited Loading

Choose a reason for hiding this comment

navidshaikh Jun 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knative-prow-robot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maximilien commented Jun 24, 2020

rhuss commented Jun 25, 2020

zhanggbj Jun 19, 2020 •

edited

Loading

zhanggbj commented Jun 21, 2020 •

edited

Loading

maximilien left a comment •

edited

Loading

navidshaikh Jun 24, 2020 •

edited

Loading

navidshaikh Jun 24, 2020 •

edited

Loading