Skip to content

Get revision and service listing skeletons in.#2

Merged
sixolet merged 7 commits intoknative:masterfrom
sixolet:command
Jan 22, 2019
Merged

Get revision and service listing skeletons in.#2
sixolet merged 7 commits intoknative:masterfrom
sixolet:command

Conversation

@sixolet
Copy link
Copy Markdown
Contributor

@sixolet sixolet commented Jan 11, 2019

This adds the kn program, with basic commands for service and revision listing
in.

kn service list
kn revision list

They use the genericclioptions library from kubernetes to support jsonpath,
yaml, and json output. The default is to just list the names of the objects, for
now, until we add in some of the kubectl-based libraries for creating tables in
another commit.

This is deliberately bare-bones; I am looking to get the basic skeletons of how
this repository is laid out down.

No tests yet. Soon.

This adds the kn program, with basic commands for service and revision listing
in.

kn service list
kn revision list

They use the genericclioptions library from kubernetes to support jsonpath,
yaml, and json output. The default is to just list the names of the objects, for
now, until we add in some of the kubectl-based libraries for creating tables in
another commit.

This is deliberately bare-bones; I am looking to get the basic skeletons of how
this repository is laid out down.

No tests yet. Soon.
@knative-prow-robot knative-prow-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jan 11, 2019
@sixolet sixolet requested a review from cppforlife January 11, 2019 02:05
@cppforlife
Copy link
Copy Markdown

@sixolet what do you think about pattern a lot of go CLIs go with: have a cmd/kn/ directory with main.go and pkg/kn/commands/ that contains implementation of commands (similar to https://github.com/kubernetes/kubernetes/blob/master/cmd/kubectl/kubectl.go)

@cppforlife
Copy link
Copy Markdown

i think we should also figure out how to move some of this stuff into objects instead of global variables. i've heard for example from riff folks that they want to use CLI commands as a library as well to add more of their own commands. might be a bit cleaner.

we should also figure out how to only have one exit point (one os.Exit(1)) and propagate errors instead of just existing.

@sixolet
Copy link
Copy Markdown
Contributor Author

sixolet commented Jan 14, 2019

That structure of files sounds fine and I'll make this PR so; this is just the structure generated for me by the cobra commandline, rather than something I decided was best.

I'll look at the exiting and non-global suggestions and see if they fit in this PR or a separate follow-on PR or two.

@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 15, 2019
@sixolet
Copy link
Copy Markdown
Contributor Author

sixolet commented Jan 15, 2019

K, I think making stuff less global is going to wait; the examples I can find of Cobra best practices all have their commands global and things initialized using module init functions. Let's have conversation about how to unglobalize stuff after this is in?

Comment thread pkg/kn/commands/service_list.go Outdated
// use the current context in kubeconfig
config, err := clientcmd.BuildConfigFromFlags("", kubeCfgFile)
if err != nil {
panic(err.Error())
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return err here

Comment thread pkg/kn/commands/revision_list.go Outdated
// use the current context in kubeconfig
config, err := clientcmd.BuildConfigFromFlags("", kubeCfgFile)
if err != nil {
panic(err.Error())
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return err here

@cppforlife
Copy link
Copy Markdown

I think making stuff less global is going to wait

ok. will wait for that.

the examples I can find of Cobra best practices all have their commands global and things initialized using module init functions

knctl does it the same way as kubectl

ultimately all commands are combined into root cmd:

https://github.com/cppforlife/knctl/blob/master/pkg/knctl/cmd/knctl.go

@vdemeester
Copy link
Copy Markdown

Agreeing with @cppforlife on the non global variable thing. In docker/cli we're doing something close to what kubectl does : pull, combined in commands.go.

That said, we can iterate in follow-ups if needed 👼

@cppforlife
Copy link
Copy Markdown

/lgtm as a starting point.

No tests yet. Soon.

i think this should be next. writing some unit tests may actually change some of the structure.

@cppforlife
Copy link
Copy Markdown

/approve

Copy link
Copy Markdown

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 22, 2019
@knative-prow-robot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cppforlife, sixolet, vdemeester

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

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

@sixolet sixolet merged commit 38932b1 into knative:master Jan 22, 2019
eletonia added a commit to eletonia/client that referenced this pull request Mar 8, 2021
# This is the 1st commit message:
adding BUILT-IN SOURCE column for kn source list-types

# The commit message knative#2 will be skipped:

#	changing list test to check for BUILT-IN SOURCE column

# The commit message knative#3 will be skipped:

#	changing e2e source list test to check for BUILT-IN SOURCE column

# The commit message knative#4 will be skipped:

#	adding CHANGELOG entry

# The commit message knative#5 will be skipped:

#	kn source list-types: changing BUILT-IN SOURCE to BUILT-IN and moving DESCRIPTION column to the end

# The commit message knative#6 will be skipped:

#	changing BUILT-IN SOURCE to BUILT-IN in changelog

# The commit message knative#7 will be skipped:

#	Update CHANGELOG.adoc
#
#	Co-authored-by: David Simansky <dsimansk@redhat.com>

# The commit message knative#8 will be skipped:

#	kn source list-types: changing column header to S, values to X, and moving to second column

# The commit message knative#9 will be skipped:

#	fixing CHANGELOG merge conflict
knative-prow-robot pushed a commit that referenced this pull request Mar 9, 2021
* # This is a combination of 9 commits.
# This is the 1st commit message:
adding BUILT-IN SOURCE column for kn source list-types

# The commit message #2 will be skipped:

#	changing list test to check for BUILT-IN SOURCE column

# The commit message #3 will be skipped:

#	changing e2e source list test to check for BUILT-IN SOURCE column

# The commit message #4 will be skipped:

#	adding CHANGELOG entry

# The commit message #5 will be skipped:

#	kn source list-types: changing BUILT-IN SOURCE to BUILT-IN and moving DESCRIPTION column to the end

# The commit message #6 will be skipped:

#	changing BUILT-IN SOURCE to BUILT-IN in changelog

# The commit message #7 will be skipped:

#	Update CHANGELOG.adoc
#
#	Co-authored-by: David Simansky <dsimansk@redhat.com>

# The commit message #8 will be skipped:

#	kn source list-types: changing column header to S, values to X, and moving to second column

# The commit message #9 will be skipped:

#	fixing CHANGELOG merge conflict

* adding BUILT-IN SOURCE column for kn source list-types
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants