Skip to content
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

Harmonize list headers for "trigger list", "cronjob list" and "apiserver list" #567

Closed
rhuss opened this issue Dec 17, 2019 · 4 comments · Fixed by #658
Closed

Harmonize list headers for "trigger list", "cronjob list" and "apiserver list" #567

rhuss opened this issue Dec 17, 2019 · 4 comments · Fixed by #658
Assignees
Labels
kind/feature New feature or request

Comments

@rhuss
Copy link
Contributor

rhuss commented Dec 17, 2019

Actually the columns wrt to Sink / Subscription are different:

  • CronJon and Apiserver source display the sink shortcut ('svc:myservice')
  • Trigger shows the resolved URL ("SUBSCRIBER_URL")

Also, the condition and reason columns should be the same as for services.
Should we add AGE, too ?

@rhuss rhuss added kind/feature New feature or request topic/eventing-mvp labels Dec 17, 2019
@rhuss rhuss assigned rhuss and daisy-ycguo and unassigned rhuss Feb 5, 2020
@daisy-ycguo
Copy link
Member

daisy-ycguo commented Feb 11, 2020

Current status:

$ ./kn trigger list
NAME       BROKER    SUBSCRIBER_URI                                   READY   REASON
trigger1   default   http://event-display.default.svc.cluster.local   True

$ ./kn source cronjob list
NAME              SCHEDULE      SINK                CONDITIONS   READY   REASON
my-cron-trigger   * * * * */1   svc:event-display   7 OK / 7     True

$ ./kn source apiserver list
NAME        RESOURCES        SINK                CONDITIONS   READY   REASON
k8sevents   Event:v1:false   svc:event-display   6 OK / 6     True

$ ./kn service list
NAME            URL                                                                                                                   LATEST                  AGE    CONDITIONS   READY   REASON
event-display   http://event-display-default.myclusterguoyc-5290c8c8e5797924dc1ad5d1b85b37c0-0000.au-syd.containers.appdomain.cloud   event-display-jvqmp-1   142m   3 OK / 3     True

I propose below changes:

  1. Change SUBSCRIBER_URI in trigger list to SINK as cronjob list and apiserver list
  2. Add AGE as service list

@rhuss what do you mean by Also, the condition and reason columns should be the same as for services.? I think CONDITIONS, READY and REASON are already same as service list.

@rhuss
Copy link
Contributor Author

rhuss commented Feb 11, 2020

I'm now not 100% sure anymore whether we should move from SUBSCRIBER_URI to SINK, but maybe rename SUBSCRIBER_URI to SUBSCRIBER, as in the underlying CR there are named subscriber and sink, so we should not invent our own naming for the subscriber as this very likely confuses people more than it helps.

wrt/ the conditions, ready and reason columns I'm not sure anymore what I meant. So happy to leave it as it is now (note to myself: be more precise when creating issues ;-)

Agreed to adding the age column to sources and triggers.

@daisy-ycguo
Copy link
Member

daisy-ycguo commented Feb 11, 2020

I'm now not 100% sure anymore whether we should move from SUBSCRIBER_URI to SINK, but maybe rename SUBSCRIBER_URI to SUBSCRIBER, as in the underlying CR there are named subscriber and sink, so we should not invent our own naming for the subscriber as this very likely confuses people more than it helps.

Do you want to keep the uri as subscriber or the pattern svc:event-display as subscriber, same as sources ?

wrt/ the conditions, ready and reason columns I'm not sure anymore what I meant. So happy to leave it as it is now (note to myself: be more precise when creating issues ;-)

Agreed to adding the age column to sources and triggers.

@rhuss
Copy link
Contributor Author

rhuss commented Feb 11, 2020

Do you want to keep the uri as subscriber or the pattern svc:event-display as subscriber, same as sources ?

yes please

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants