add domain list command #3

ZhuangYuZY · 2020-10-16T15:36:39Z

knative-prow-robot · 2020-10-16T15:36:50Z

Welcome @ZhuangYuZY! It looks like this is your first PR to knative-sandbox/kn-plugin-admin 🎉

rhuss

Thanks, this feature makes totally sense. Find my review comments below.

rhuss · 2020-10-20T09:32:49Z

pkg/command/domain/human_readable_flags.go

+func DomainListHandlers(h hprinters.PrintHandler) {
+	kDomainColumnDefinitions := []metav1beta1.TableColumnDefinition{
+		{Name: "Custom-Domain", Type: "string", Description: "Name of Knative custom domain.", Priority: 1},
+		{Name: "Selector", Type: "string", Description: "Selector of the Knative custom domain.", Priority: 1},


Suggested change

{Name: "Selector", Type: "string", Description: "Selector of the Knative custom domain.", Priority: 1},

{Name: "Selector", Type: "string", Description: "Selector for Knative custom domains.", Priority: 1},

agree, code changed. Thank you.

rhuss · 2020-10-20T09:36:16Z

pkg/command/domain/human_readable_flags.go

+	parts := strings.Split(strings.ReplaceAll(strings.TrimSpace(selector), ":", "="), "\n")
+	selectorForPrint := ""
+	for i, v := range parts {
+		if i == 0 && strings.Compare(v, "selector=") != 0 {


Maybe better use StartsWith instead of Compare ? Compare looks a bit strange in this context and is not easy to understand.

But maybe I just don't understand this special case for i=0. Could you please elaborate a bit on the reasoning for this handling ?

Agree, code changed, and also add some comment. Thank you.

rhuss · 2020-10-20T10:04:21Z

pkg/command/domain/human_readable_flags_test.go

+		input  string
+		output string
+	}{
+		{"normal case with one selector key value", "selector:\n  key1: value1\n", "key1=value1; "},


Could we also trim the final ; + the trailing space ? I think this looks more streamlined

You are right. Code changed. Thank you.

pkg/command/domain/human_readable_flags.go

zhanggbj · 2020-10-23T06:53:56Z

pkg/command/domain/human_readable_flags.go

@@ -0,0 +1,64 @@
+// Copyright © 2019 The Knative Authors


@ZhuangYuZY I think the license should be 2020.

agree, code change.

Hi @ZhuangYuZY , looks like this one is missed?

Hi @zhanggbj , In Conversation, the file is outdated. Please check "Files changed" tab. I already made the change of 2019 --> 2020. Thank you.

zhanggbj · 2020-10-23T06:56:37Z

pkg/command/domain/human_readable_flags_test.go

@@ -0,0 +1,41 @@
+// Copyright © 2019 The Knative Authors
+//


Same as above

agree, code changed. Thank you.

Same as above.

Hi @zhanggbj , In Conversation, the file is outdated. Please check "Files changed" tab. I already made the change of 2019 --> 2020. Thank you.

zhanggbj · 2020-10-23T06:57:53Z

pkg/command/domain/list.go

+	domainListFlags := flags.NewListPrintFlags(DomainListHandlers)
+	domainListCommand := &cobra.Command{
+		Use:   "list",
+		Short: "List custom domain",


Same as above, I think we can use domain instead of custom domain here.

How about this one, what do you think? Thanks!

Hi @zhanggbj , In Conversation, the file is outdated. Please check "Files changed" tab. Thank you.

zhanggbj · 2020-10-23T07:00:29Z

@ZhuangYuZY Thanks for the PR! I just left some minor comments. Thanks!

ZhuangYuZY · 2020-10-23T10:38:34Z

Thank you all, I will address your comment next week.

ZhuangYuZY · 2020-10-26T09:26:08Z

@rhuss and @zhanggbj thank you for your comments. All addressed.

zhanggbj · 2020-11-05T05:55:05Z

/approve
@ZhuangYuZY Thanks for the PR! Waiting for @rhuss if any further comments!

zhanggbj · 2020-11-05T12:10:02Z

pkg/command/domain/list.go

+
+	"github.com/spf13/cobra"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"knative.dev/client-contrib/plugins/admin/pkg"


Hi @ZhuangYuZY
Same situation with this new issue #11, would you please help to update the import path, otherwise it will break. Thanks!

fixed. Thank you.

zhanggbj · 2020-11-05T12:10:18Z

pkg/command/domain/list_test.go

+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	k8sfake "k8s.io/client-go/kubernetes/fake"
+	"knative.dev/client-contrib/plugins/admin/pkg"


same as above

fixed. Thank you.

zhanggbj · 2020-11-05T12:10:35Z

pkg/command/domain/list_test.go

+	"knative.dev/client-contrib/plugins/admin/pkg"
+	"knative.dev/client/pkg/util"
+
+	"knative.dev/client-contrib/plugins/admin/pkg/testutil"


same as above

fixed. Thank you.

zhanggbj · 2020-11-05T12:14:36Z

@ZhuangYuZY Sorry for requesting changes again, there's an issue filed today #11, would you please help to update the import path here? Thanks!

ZhuangYuZY · 2020-11-11T08:55:32Z

@zhanggbj import path fixed. Thank you.

knative-prow-robot · 2020-11-11T09:07:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: zhanggbj, ZhuangYuZY

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~~ [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-11-11T09:17:56Z

/lgtm

@ZhuangYuZY Thanks for the PR! Merging it

add domain list command

9e6d63e

google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Oct 16, 2020

knative-prow-robot requested review from navidshaikh and rhuss October 16, 2020 15:36

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 16, 2020

ZhuangYuZY mentioned this pull request Oct 16, 2020

As knative admin, I want to list custom domains knative/client-contrib#30

Open

rhuss reviewed Oct 20, 2020

View reviewed changes

zhanggbj reviewed Oct 23, 2020

View reviewed changes

ZhuangYuZY added 2 commits October 26, 2020 17:13

address code review comments for domain list command

942806b

address code review comment, make some comment more clear

dba14f6

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 5, 2020

zhanggbj reviewed Nov 5, 2020

View reviewed changes

fix import path as repo change

5bf664d

zhanggbj approved these changes Nov 11, 2020

View reviewed changes

knative-prow-robot assigned zhanggbj Nov 11, 2020

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

knative-prow-robot merged commit 2092070 into knative-extensions:master Nov 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add domain list command #3

add domain list command #3

ZhuangYuZY commented Oct 16, 2020 •

edited

Loading

knative-prow-robot commented Oct 16, 2020

rhuss left a comment

rhuss Oct 20, 2020

ZhuangYuZY Oct 26, 2020 •

edited

Loading

rhuss Oct 20, 2020

rhuss Oct 20, 2020

ZhuangYuZY Oct 26, 2020

rhuss Oct 20, 2020

ZhuangYuZY Oct 26, 2020

zhanggbj Oct 23, 2020

ZhuangYuZY Oct 26, 2020

zhanggbj Nov 1, 2020

ZhuangYuZY Nov 3, 2020 •

edited

Loading

zhanggbj Oct 23, 2020

ZhuangYuZY Oct 26, 2020

zhanggbj Nov 1, 2020

ZhuangYuZY Nov 3, 2020 •

edited

Loading

zhanggbj Oct 23, 2020

zhanggbj Nov 1, 2020

ZhuangYuZY Nov 3, 2020 •

edited

Loading

zhanggbj commented Oct 23, 2020

ZhuangYuZY commented Oct 23, 2020 •

edited

Loading

ZhuangYuZY commented Oct 26, 2020

zhanggbj commented Nov 5, 2020

zhanggbj Nov 5, 2020

ZhuangYuZY Nov 11, 2020

zhanggbj Nov 5, 2020

ZhuangYuZY Nov 11, 2020

zhanggbj Nov 5, 2020

ZhuangYuZY Nov 11, 2020

zhanggbj commented Nov 5, 2020

ZhuangYuZY commented Nov 11, 2020

knative-prow-robot commented Nov 11, 2020

zhanggbj commented Nov 11, 2020

	{Name: "Selector", Type: "string", Description: "Selector of the Knative custom domain.", Priority: 1},
	{Name: "Selector", Type: "string", Description: "Selector for Knative custom domains.", Priority: 1},

add domain list command #3

add domain list command #3

Conversation

ZhuangYuZY commented Oct 16, 2020 • edited Loading

knative-prow-robot commented Oct 16, 2020

rhuss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZhuangYuZY Oct 26, 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

Choose a reason for hiding this comment

ZhuangYuZY Nov 3, 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

ZhuangYuZY Nov 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZhuangYuZY Nov 3, 2020 • edited Loading

Choose a reason for hiding this comment

zhanggbj commented Oct 23, 2020

ZhuangYuZY commented Oct 23, 2020 • edited Loading

ZhuangYuZY commented Oct 26, 2020

zhanggbj commented Nov 5, 2020

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 commented Nov 5, 2020

ZhuangYuZY commented Nov 11, 2020

knative-prow-robot commented Nov 11, 2020

zhanggbj commented Nov 11, 2020

ZhuangYuZY commented Oct 16, 2020 •

edited

Loading

ZhuangYuZY Oct 26, 2020 •

edited

Loading

ZhuangYuZY Nov 3, 2020 •

edited

Loading

ZhuangYuZY Nov 3, 2020 •

edited

Loading

ZhuangYuZY Nov 3, 2020 •

edited

Loading

ZhuangYuZY commented Oct 23, 2020 •

edited

Loading