feature: Initialize kn admin plugin #2

zhanggbj · 2020-02-18T15:04:06Z

Hi All,

This kn-admin plugin is designed to help administrators or operators better manage a Knative platform installation with kn CLI.

This is an initial commit. More details in the proposal: https://docs.google.com/document/d/1oD5P4WQZs39GIr_4ezcURgRCuUx8dXrcCi3N7GKmp5c/edit#

Thanks!

knative-prow-robot · 2020-02-18T15:04:21Z

Welcome @zhanggbj! It looks like this is your first PR to knative/client-contrib 🎉

knative-prow-robot · 2020-02-18T15:04:22Z

Hi @zhanggbj. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

rhuss

Thanks a lot ! It's a crazy busy week for me (and next week I have PTO :(, so I hope I can bring it forward this week still.

sorry for the delay ...

zhanggbj · 2020-02-21T06:32:25Z

@rhuss
Don't worry, take your time:-)

rhuss · 2020-02-23T07:14:18Z

@zhanggbj sorry, I couldn't make it before my PTO (and I'm heading now off to the Alpes with little connectivity). I will be back the first week in March and get your PRs merged then is on the top of my priority list.

Apologies, and have a nice week !

maximilien · 2020-02-25T18:48:40Z

/ok-to-test

maximilien · 2020-02-26T20:36:14Z

Hi, @zhanggbj, so all the tests are failing :(

I’ll leave some comments as well. But please fix this ^^^ first. Thanks.

maximilien

Hi, @zhanggbj, thanks for the contribution. However, few things TODO:

Please address comments
Add unit tests
Try to at least add one e2e test if possible. OK to have that as a separate PR
Since this PR has the dependency (1000s of files). It would be easier to have two PRs. But OK with one if that simple for you
Ensure all UTs are passing

plugins/kn-admin/README.md

plugins/kn-admin/cmd/private-registry/private-registry.go

plugins/kn-admin/main.go

plugins/kn-admin/hack/build.sh

plugins/kn-admin/core/root.go

knative-prow-robot · 2020-03-03T14:48:14Z

@zhanggbj: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-knative-client-contrib-go-coverage	`ee9b4cc`	link	`/test pull-knative-client-contrib-go-coverage`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

zhanggbj · 2020-03-03T14:49:35Z

@maximilien Thank you for you review!

I've address your most of your comments and left one comment (add version in next PR), would you please help to take a review, thx!

I check the test failure, looks like it doesn't relate do plugin code, it cannot find the proper test file to execute. For e.g.

+ ./test/presubmit-tests.sh --integration-tests
./test/presubmit-tests.sh: line 37: ./test/../vendor/knative.dev/test-infra/scripts/presubmit-tests.sh: No such file or directory
./test/presubmit-tests.sh: line 40: main: command not found
+ EXIT_VALUE=127
+ set +o xtrace

maximilien · 2020-03-05T23:09:45Z

Hi, @zhanggbj I think you need to add the test files in your PR. Doing that too for my PR #4.

I guess which ever gets merged first will have these files set and we can resolve conflicts in subsequent PRs.

Please do that and get all green and I will do a final pass. Thanks.

rhuss · 2020-03-24T16:40:02Z

go.mod

@@ -0,0 +1,14 @@
+module github.com/knative/client-contrib


there should be no top-level go.mod but each individual plugin uses its own dependency tree. the same is true for the vendor/ dir. See the comments to #5 for the reasoning and how I suggest to progress.

@rhuss sure, I'm working on it, will move vendor to the admin own structure and resubmit.

rhuss · 2020-04-07T09:02:44Z

/retest

mattmoor

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

rhuss · 2020-04-14T09:10:49Z

The following users are mentioned in OWNERS file(s) but are not members of the knative org.

Once all users have been added as members of the org, you can trigger verification by writing /verify-owners in a comment.

zhanggbj

OWNERS

@zhanggbj I asked the Knative admins to add you to the GitHub orga

zhanggbj · 2020-04-14T09:21:03Z

Hi @rhuss @maximilien,
I've restructured kn-admin to align with the hello plugin, including folders/vendors/Reame/LICENSE/etc. would you please help to take a review? Thanks!

This is an initial PR for kn-admin and I still have a lot of TODOs in my list. I'll raise new PRs for review later as this already got a little bit hard to review:-)

Fill up unit test and e2e test for kn-admin [kn-admin] Add Unit Test and e2e test #14
New features (detailes in the docs folder)

rhuss · 2020-04-14T12:53:10Z

OWNERS

@@ -4,6 +4,7 @@ approvers:
 - rhuss
 - maximilien
 - navidshaikh
+- zhanggbj


I wonder whether we could add a OWNERS fils within the plugins/admin directory ? I think this would be nice as it highlights the responsibilities and point-of-contact better.

I think prow supports this, let me check ...

Sounds good to me. Thanks a lot and I've joined Knative org.

rhuss

Thanks a lot ! (also for your patience ;-)

Here's a first round of comments concerning UX aspects. In generall it looks good to me so far, except that I would think a bit more about the commands / option naming.

plugins/admin/README.adoc

plugins/admin/core/root.go

rhuss · 2020-04-15T09:17:21Z

@zhanggbj btw, I would be happy to already commit the PR and then we can continue to work on it later.

- Added kn-admin plugin - Added domain set - Added private registry enable

- To align with hello example plugin, restruct and add missing part - Add admin plugin maintainer in OWNERS

mattmoor · 2020-04-19T11:28:00Z

plugins/admin/vendor/github.com/hashicorp/hcl/hcl/scanner/scanner.go

+}
+
+// scanMantissa scans the mantissa beginning from the rune. It returns the next
+// non decimal rune. It's used to determine wheter it's a fraction or exponent.


Found potential misspelling(s):

Suggested change

// non decimal rune. It's used to determine wheter it's a fraction or exponent.

// non decimal rune. It's used to determine whether it's a fraction or exponent.

mattmoor · 2020-04-19T11:28:01Z

plugins/admin/vendor/github.com/hashicorp/hcl/json/scanner/scanner.go

+}
+
+// scanMantissa scans the mantissa beginning from the rune. It returns the next
+// non decimal rune. It's used to determine wheter it's a fraction or exponent.


Found potential misspelling(s):

Suggested change

// non decimal rune. It's used to determine wheter it's a fraction or exponent.

// non decimal rune. It's used to determine whether it's a fraction or exponent.

mattmoor · 2020-04-19T11:28:01Z

plugins/admin/vendor/github.com/spf13/afero/lstater.go

+// filesystems saying so.
+// It will call Lstat if the filesystem iself is, or it delegates to, the os filesystem.
+// Else it will call Stat.
+// In addtion to the FileInfo, it will return a boolean telling whether Lstat was called or not.


Found potential misspelling(s):

Suggested change

// In addtion to the FileInfo, it will return a boolean telling whether Lstat was called or not.

// In addition to the FileInfo, it will return a boolean telling whether Lstat was called or not.

mattmoor · 2020-04-19T11:28:01Z

plugins/admin/vendor/github.com/spf13/jwalterweatherman/README.md

+I really wanted a very straightforward library that could seamlessly do
+the following things.
+
+1. Replace all the println, printf, etc statements thoughout my code with


Found potential misspelling(s):

Suggested change

1. Replace all the println, printf, etc statements thoughout my code with

1. Replace all the println, printf, etc statements throughout my code with

mattmoor · 2020-04-19T11:28:02Z

plugins/admin/vendor/golang.org/x/net/idna/idna9.0.0.go

+	}
+}
+
+// StrictDomainName limits the set of permissable ASCII characters to those


Found potential misspelling(s):

Suggested change

// StrictDomainName limits the set of permissable ASCII characters to those

// StrictDomainName limits the set of permissible ASCII characters to those

mattmoor · 2020-04-19T11:28:05Z

plugins/admin/vendor/gopkg.in/ini.v1/key.go

@@ -0,0 +1,801 @@
+// Copyright 2014 Unknwon


Found potential misspelling(s):

Suggested change

// Copyright 2014 Unknwon

// Copyright 2014 Unknown

mattmoor · 2020-04-19T11:28:05Z

plugins/admin/vendor/gopkg.in/ini.v1/parser.go

@@ -0,0 +1,526 @@
+// Copyright 2015 Unknwon


Found potential misspelling(s):

Suggested change

// Copyright 2015 Unknwon

// Copyright 2015 Unknown

mattmoor · 2020-04-19T11:28:05Z

plugins/admin/vendor/gopkg.in/ini.v1/parser.go

+
+	// NOTE: Iterate and increase `currentPeekSize` until
+	// the size of the parser buffer is found.
+	// TODO(unknwon): When Golang 1.10 is the lowest version supported, replace with `parserBufferSize := p.buf.Size()`.


Found potential misspelling(s):

Suggested change

// TODO(unknwon): When Golang 1.10 is the lowest version supported, replace with `parserBufferSize := p.buf.Size()`.

// TODO(unknown): When Golang 1.10 is the lowest version supported, replace with `parserBufferSize := p.buf.Size()`.

mattmoor · 2020-04-19T11:28:06Z

plugins/admin/vendor/gopkg.in/ini.v1/section.go

@@ -0,0 +1,256 @@
+// Copyright 2014 Unknwon


Found potential misspelling(s):

Suggested change

// Copyright 2014 Unknwon

// Copyright 2014 Unknown

mattmoor · 2020-04-19T11:28:06Z

plugins/admin/vendor/gopkg.in/ini.v1/struct.go

@@ -0,0 +1,603 @@
+// Copyright 2014 Unknwon


Found potential misspelling(s):

Suggested change

// Copyright 2014 Unknwon

// Copyright 2014 Unknown

mattmoor

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

mattmoor · 2020-04-19T11:28:38Z

plugins/admin/vendor/github.com/hashicorp/hcl/decoder.go

+	// we need to un-flatten the ast enough to decode
+	newNode := &ast.ObjectItem{
+		Keys: []*ast.ObjectKey{
+			&ast.ObjectKey{


Format Go code:

Suggested change

&ast.ObjectKey{

{

mattmoor · 2020-04-19T11:28:39Z

plugins/admin/vendor/github.com/modern-go/concurrent/log.go

+	"os"
+	"log"


Format Go code:

Suggested change

"os"

"log"

mattmoor · 2020-04-19T11:28:39Z

plugins/admin/vendor/github.com/modern-go/concurrent/log.go

+import (
+	"os"
+	"log"
+	"io/ioutil"


Format Go code:

Suggested change

"io/ioutil"

"io/ioutil"

"log"

"os"

mattmoor · 2020-04-19T11:28:39Z

plugins/admin/vendor/github.com/modern-go/concurrent/log.go

+// ErrorLogger is used to print out error, can be set to writer other than stderr
+var ErrorLogger = log.New(os.Stderr, "", 0)
+
+// InfoLogger is used to print informational message, default to off


Format Go code:

Suggested change

// InfoLogger is used to print informational message, default to off

// InfoLogger is used to print informational message, default to off

var InfoLogger = log.New(ioutil.Discard, "", 0)

mattmoor · 2020-04-19T11:28:39Z

plugins/admin/vendor/github.com/modern-go/concurrent/unbounded_executor.go

+
+import (
+	"context"
+	"fmt"


Format Go code:

Suggested change

"fmt"

"fmt"

"reflect"

mattmoor · 2020-04-19T11:28:42Z

plugins/admin/vendor/gopkg.in/yaml.v2/resolve.go

+					return yaml_INT_TAG, uintv
+				}
+			} else if strings.HasPrefix(plain, "-0b") {
+				intv, err := strconv.ParseInt("-" + plain[3:], 2, 64)


Format Go code:

Suggested change

intv, err := strconv.ParseInt("-" + plain[3:], 2, 64)

intv, err := strconv.ParseInt("-"+plain[3:], 2, 64)

mattmoor · 2020-04-19T11:28:43Z

plugins/admin/vendor/gopkg.in/yaml.v2/sorter.go

+		var ai, bi int
+		var an, bn int64
+		if ar[i] == '0' || br[i] == '0' {
+			for j := i-1; j >= 0 && unicode.IsDigit(ar[j]); j-- {


Format Go code:

Suggested change

for j := i-1; j >= 0 && unicode.IsDigit(ar[j]); j-- {

for j := i - 1; j >= 0 && unicode.IsDigit(ar[j]); j-- {

mattmoor · 2020-04-19T11:28:44Z

plugins/admin/vendor/k8s.io/apimachinery/pkg/apis/meta/v1/meta.go

+func (meta *ObjectMeta) SetDeletionTimestamp(deletionTimestamp *Time) {
+	meta.DeletionTimestamp = deletionTimestamp
+}
+func (meta *ObjectMeta) GetDeletionGracePeriodSeconds() *int64 { return meta.DeletionGracePeriodSeconds }


Format Go code:

Suggested change

func (meta *ObjectMeta) GetDeletionGracePeriodSeconds() *int64 { return meta.DeletionGracePeriodSeconds }

func (meta *ObjectMeta) GetDeletionGracePeriodSeconds() *int64 {

return meta.DeletionGracePeriodSeconds

}

mattmoor · 2020-04-19T11:28:44Z

plugins/admin/vendor/k8s.io/apimachinery/pkg/fields/selector.go

+func (n nothingSelector) Matches(_ Fields) bool                                      { return false }
+func (n nothingSelector) Empty() bool                                                { return false }
+func (n nothingSelector) String() string                                             { return "" }
+func (n nothingSelector) Requirements() Requirements                                 { return nil }
+func (n nothingSelector) DeepCopySelector() Selector                                 { return n }
+func (n nothingSelector) RequiresExactMatch(field string) (value string, found bool) { return "", false }
+func (n nothingSelector) Transform(fn TransformFunc) (Selector, error)               { return n, nil }


Format Go code:

Suggested change

func (n nothingSelector) Matches(_ Fields) bool { return false }

func (n nothingSelector) Empty() bool { return false }

func (n nothingSelector) String() string { return "" }

func (n nothingSelector) Requirements() Requirements { return nil }

func (n nothingSelector) DeepCopySelector() Selector { return n }

func (n nothingSelector) RequiresExactMatch(field string) (value string, found bool) { return "", false }

func (n nothingSelector) Transform(fn TransformFunc) (Selector, error) { return n, nil }

func (n nothingSelector) Matches(_ Fields) bool { return false }

func (n nothingSelector) Empty() bool { return false }

func (n nothingSelector) String() string { return "" }

func (n nothingSelector) Requirements() Requirements { return nil }

func (n nothingSelector) DeepCopySelector() Selector { return n }

func (n nothingSelector) RequiresExactMatch(field string) (value string, found bool) {

return "", false

}

func (n nothingSelector) Transform(fn TransformFunc) (Selector, error) { return n, nil }

mattmoor · 2020-04-19T11:28:45Z

plugins/admin/vendor/k8s.io/apimachinery/pkg/labels/selector.go

+func (n nothingSelector) Matches(_ Labels) bool                                      { return false }
+func (n nothingSelector) Empty() bool                                                { return false }
+func (n nothingSelector) String() string                                             { return "" }
+func (n nothingSelector) Add(_ ...Requirement) Selector                              { return n }
+func (n nothingSelector) Requirements() (Requirements, bool)                         { return nil, false }
+func (n nothingSelector) DeepCopySelector() Selector                                 { return n }
+func (n nothingSelector) RequiresExactMatch(label string) (value string, found bool) { return "", false }


Format Go code:

Suggested change

func (n nothingSelector) Matches(_ Labels) bool { return false }

func (n nothingSelector) Empty() bool { return false }

func (n nothingSelector) String() string { return "" }

func (n nothingSelector) Add(_ ...Requirement) Selector { return n }

func (n nothingSelector) Requirements() (Requirements, bool) { return nil, false }

func (n nothingSelector) DeepCopySelector() Selector { return n }

func (n nothingSelector) RequiresExactMatch(label string) (value string, found bool) { return "", false }

func (n nothingSelector) Matches(_ Labels) bool { return false }

func (n nothingSelector) Empty() bool { return false }

func (n nothingSelector) String() string { return "" }

func (n nothingSelector) Add(_ ...Requirement) Selector { return n }

func (n nothingSelector) Requirements() (Requirements, bool) { return nil, false }

func (n nothingSelector) DeepCopySelector() Selector { return n }

func (n nothingSelector) RequiresExactMatch(label string) (value string, found bool) {

return "", false

}

zhanggbj · 2020-04-19T11:31:35Z

@rhuss
I've addressed all your comments and would you please help to take a look? Thanks!
And this is an initial PR, i think there're still a lot of todos for new subcommand and for existed domain and registry CRUD actions, and we can discuss later after this PR is merged and easy for collaboration.

rhuss

Thanks a lot ! Congrats to the first plugin merged ever to knative/client-contrib ;-) 👏

Let's continue to work on it to provide even more features. Also, I'm going to open an issue to collect more ideas for what might be interesting to add, so if we have kind of a complete overview we can align the command structure, too.

/lgtm
/approve

knative-prow-robot · 2020-04-20T08:17:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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:

~~OWNERS~~ [rhuss]

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

* Initialize kn-admin plugin - Added kn-admin plugin - Added domain set - Added private registry enable * Restruct kn-admin plugin - To align with hello example plugin, restruct and add missing part - Add admin plugin maintainer in OWNERS

knative-prow-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 18, 2020

knative-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Feb 18, 2020

knative-prow-robot requested review from cppforlife and evankanderson February 18, 2020 15:04

zhanggbj force-pushed the init_kn_admin branch from 75cd6a5 to 2c377f7 Compare February 18, 2020 15:04

zhanggbj requested review from rhuss, maximilien, duglin and sixolet February 18, 2020 15:05

zhanggbj force-pushed the init_kn_admin branch from 2c377f7 to 2c72aa5 Compare February 19, 2020 14:45

rhuss reviewed Feb 19, 2020

View reviewed changes

knative-prow-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 25, 2020

maximilien suggested changes Feb 26, 2020

View reviewed changes

zhanggbj force-pushed the init_kn_admin branch from 2c72aa5 to ee9b4cc Compare March 3, 2020 14:46

rhuss reviewed Mar 24, 2020

View reviewed changes

zhanggbj force-pushed the init_kn_admin branch from ee9b4cc to 0c02219 Compare April 12, 2020 15:23

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

zhanggbj changed the title ~~Initialize kn admin plugin~~ feature: Initialize kn admin plugin Apr 12, 2020

mattmoor reviewed Apr 12, 2020

View reviewed changes

rhuss reviewed Apr 14, 2020

View reviewed changes

Gong Zhang added 2 commits April 19, 2020 19:20

Initialize kn-admin plugin

b01f0ad

- Added kn-admin plugin - Added domain set - Added private registry enable

Restruct kn-admin plugin

47f710a

- To align with hello example plugin, restruct and add missing part - Add admin plugin maintainer in OWNERS

zhanggbj force-pushed the init_kn_admin branch from 0c02219 to 47f710a Compare April 19, 2020 11:27

mattmoor reviewed Apr 19, 2020

View reviewed changes

rhuss approved these changes Apr 20, 2020

View reviewed changes

knative-prow-robot assigned rhuss Apr 20, 2020

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

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

knative-prow-robot merged commit 79925ae into knative:master Apr 20, 2020

	// non decimal rune. It's used to determine wheter it's a fraction or exponent.
	// non decimal rune. It's used to determine whether it's a fraction or exponent.

	// In addtion to the FileInfo, it will return a boolean telling whether Lstat was called or not.
	// In addition to the FileInfo, it will return a boolean telling whether Lstat was called or not.

	1. Replace all the println, printf, etc statements thoughout my code with
	1. Replace all the println, printf, etc statements throughout my code with

	// StrictDomainName limits the set of permissable ASCII characters to those
	// StrictDomainName limits the set of permissible ASCII characters to those

	// TODO(unknwon): When Golang 1.10 is the lowest version supported, replace with `parserBufferSize := p.buf.Size()`.
	// TODO(unknown): When Golang 1.10 is the lowest version supported, replace with `parserBufferSize := p.buf.Size()`.

	// InfoLogger is used to print informational message, default to off
	// InfoLogger is used to print informational message, default to off
	var InfoLogger = log.New(ioutil.Discard, "", 0)

	intv, err := strconv.ParseInt("-" + plain[3:], 2, 64)
	intv, err := strconv.ParseInt("-"+plain[3:], 2, 64)

	for j := i-1; j >= 0 && unicode.IsDigit(ar[j]); j-- {
	for j := i - 1; j >= 0 && unicode.IsDigit(ar[j]); j-- {

feature: Initialize kn admin plugin #2

feature: Initialize kn admin plugin #2

Conversation

zhanggbj commented Feb 18, 2020

knative-prow-robot commented Feb 18, 2020

knative-prow-robot commented Feb 18, 2020

rhuss left a comment

Choose a reason for hiding this comment

zhanggbj commented Feb 21, 2020

rhuss commented Feb 23, 2020

maximilien commented Feb 25, 2020

maximilien commented Feb 26, 2020

maximilien left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Mar 3, 2020 • edited Loading

zhanggbj commented Mar 3, 2020

maximilien commented Mar 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhuss commented Apr 7, 2020

mattmoor left a comment

Choose a reason for hiding this comment

rhuss commented Apr 14, 2020

zhanggbj commented Apr 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhuss left a comment

Choose a reason for hiding this comment

rhuss commented Apr 15, 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

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

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

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 commented Apr 19, 2020

rhuss left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Apr 20, 2020

knative-prow-robot commented Mar 3, 2020 •

edited

Loading

zhanggbj commented Apr 14, 2020 •

edited

Loading