-
Notifications
You must be signed in to change notification settings - Fork 261
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
feat: List available channel types #1027
feat: List available channel types #1027
Conversation
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.
@Kaustubh-pande: 2 warnings.
In response to this:
Description
List available channel types in the cluster which can be used for creating channels.
Changes
Add channel list-types
Reference
Fixes #979
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.
Hi @Kaustubh-pande. 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 Once the patch is verified, the new status will be reflected by the 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. |
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.
Thanks for contribution. Will do a full review. Let’s get the tests running first.
/ok-to-test
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 fine to me, but would love to have some review from @navidshaikh , too, as he did the source list-types (and this here is quite similar).
My only suggestion would be whether we might want reconsider the list-types
name to simplify to types
(although this would break our noun verb schema) But see the comment below.
@@ -31,4 +31,5 @@ kn channel COMMAND | |||
* [kn channel delete](kn_channel_delete.md) - Delete a channel | |||
* [kn channel describe](kn_channel_describe.md) - Show details of a channel | |||
* [kn channel list](kn_channel_list.md) - List channels | |||
* [kn channel list-types](kn_channel_list-types.md) - List channel types |
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.
While just thinking about this, why not just kn channel types
? I know this violates the schema, but it looks imo nice (same for kn source list-types
). At the end "list-types" is also not a verb but an artificial word construct.
wdyt @navidshaikh @maximilien ?
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 liked that. I'd keep it list-types
for this PR and change for source and channel together in a subsequent PR (keeping list-types as an alias for source). cc @Kaustubh-pande
@@ -26,6 +26,7 @@ import ( | |||
"k8s.io/apimachinery/pkg/runtime/schema" | |||
dynamicfake "k8s.io/client-go/dynamic/fake" | |||
eventingv1beta1 "knative.dev/eventing/pkg/apis/eventing/v1beta1" | |||
messagingv1beta1 "knative.dev/eventing/pkg/apis/messaging/v1beta1" |
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 wonder whether we should already go to v1
API (and then change eventing also to v1 in a subsequent PR).
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.
lets keep it at v1beta1 to also support working with earlier eventing installs?
/assign daisy-ycguo |
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.
neat! There are a few suggestion as to which fields to print in the columns, we can remove Group and Version columns, and replace them with NAME
and DESCRIPTION
to align with kn source list-types
.
@@ -26,6 +26,7 @@ import ( | |||
"k8s.io/apimachinery/pkg/runtime/schema" | |||
dynamicfake "k8s.io/client-go/dynamic/fake" | |||
eventingv1beta1 "knative.dev/eventing/pkg/apis/eventing/v1beta1" | |||
messagingv1beta1 "knative.dev/eventing/pkg/apis/messaging/v1beta1" |
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.
lets keep it at v1beta1 to also support working with earlier eventing installs?
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.
Thank you for your contribution, @Kaustubh-pande. I have a few comments. Would you address them and update a new version ?
/retest |
1 similar comment
/retest |
e1b6fcb
to
3e2ffb7
Compare
/retest |
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.
lgtm, just a minor nit for the error message and please add the CHANGELOG entry ?
/test pull-knative-client-integration-tests |
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.
LGTM.
After tests pass, I'm OK to merge it. Thank you for your contribution.
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.
/lgtm
/approve
2729f7b
to
dd32027
Compare
/retest |
/retest |
The following is the coverage report on the affected files.
|
/retest |
/retest
|
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.
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: daisy-ycguo, Kaustubh-pande, maximilien, navidshaikh 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 |
/retest |
* add channel list-types * add kn channel list-types doc
Description
List available channel types in the cluster which can be used for creating channels.
Changes
Add channel list-types
Reference
Fixes #979