-
Notifications
You must be signed in to change notification settings - Fork 263
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feature(service describe): Output of service details #252
Conversation
@sixolet @cppforlife @maximilien All features are now implemented, please have a look at the updated issue description for all data that is shown. The focus was to show everything which also can be provided with What is missing is the unit test. However, because it really depends highly on the output to be shown I wanted to wait for first feedback if the current format is ok. I would then also do the unit tests on then KnClient level, i.e. using a mock implementation of the KnClient interface (which needs to be introduced). Or is this something which we should leave for another PR? (this one is already huge). In that case, I would make a simplified test here (to keep the code coverage test happy), and work on another PR for the full coverage including mock support for KnClient. |
8858eee
to
de38225
Compare
`kn service show` mimics `kubectl describe <sth>` as that it output detail information in human readable output. A future extension should add machine readable output with `-o` to export a single object in json/yaml (that is missing from `kn service list`) This command shows information about the service itself, but also a summary about the associated revisions. It also knows about scaling and concurrency options. Options: `--details` : Print more information. By default only are shorter summary is shown
…) + machine readable output with -o
/retest |
@sixolet @cppforlife @maximilien @navidshaikh Uff, finally I think the PR is ready for prime time. Some tests have been added (coverage 84%), please have another look at the PR. 'would be super awesome to get it merged until Friday before I head off to PTO and the PR would lie around for another three weeks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is good to go.
@@ -47,7 +45,7 @@ func TestServiceCreateImageMock(t *testing.T) { | |||
r.GetService("foo", getServiceWithUrl("foo", "http://foo.example.com"), nil) | |||
|
|||
// Testing: | |||
output, err := executeCommand(client, "create", "foo", "--image", "gcr.io/foo/bar:baz") | |||
output, err := executeServiceCommand(client, "create", "foo", "--image", "gcr.io/foo/bar:baz") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
cmd.SetArgs(args) | ||
cmd.SetOutput(output) | ||
err := cmd.Execute() | ||
return output.String(), err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this order. ❤️
/lint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@navidshaikh: 15 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.
The following is the coverage report on pkg/.
|
/lint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rhuss: 0 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, thanks Roland for working on multiple rounds for this feature 👍
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: navidshaikh, rhuss 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:
Approvers can indicate their approval by writing |
/lgtm |
kn service describe
mimicskubectl describe <sth>
as that it output detail information in human-readable output. It also adds the possibility to print the service in machine-readable output e.g. via-o yaml
. This is a difference tokubectl describe
but necessary askn list -o yaml
will always print a list, not a single object. You can get the single object now withkn describe -o yaml
, which also makes for a clear separation between list operations (multiple entities always) and single entity operation (one entity only). Hopefully, this is less confusing thankubectl get
operation which has either list or single entity output depending on arguments.This command shows information about the service itself, but also a summary of the associated revisions.
kn service describe
has the following features:--details
more information can be printed:-o yaml|json|...
or--template
of this single serviceThis PR is a subset of and replacement for #75. There is no color option included nor any hipster stuff 🤓
Examle output
kn service describe hello
kn service describe hello --details