Skip to content
This repository has been archived by the owner on May 6, 2022. It is now read-only.

don't list plans twice (svcat describe class --traverse) #2026

Merged
merged 3 commits into from May 15, 2018

Conversation

jboyd01
Copy link
Contributor

@jboyd01 jboyd01 commented May 9, 2018

When you describe a class with --traverse option, the plans are listed twice:

$ svcat describe class user-provided-service --traverse     
  Name:          user-provided-service                 
  Description:   A user provided service               
  UUID:          4f6e6cf6-ffdd-425f-a2c7-3c9258ad2468  
  Status:        Deprecated                            
  Tags:                                                
  Broker:        ups-broker                            

Plans:
   NAME           DESCRIPTION        
+---------+-------------------------+
  default   Sample plan description  

Broker:
  Name:     ups-broker  
  Status:   Ready       

Plans:
   NAME           DESCRIPTION        
+---------+-------------------------+
  default   Sample plan description  

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 9, 2018
@jboyd01 jboyd01 requested a review from carolynvs May 9, 2018 19:06
@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label May 9, 2018
@jberkhahn
Copy link
Contributor

jberkhahn commented May 9, 2018

Seems simple enough.

Are we sure that we always want to print the plans? At that point, traverse is only adding the broker information. I think you might want to remove the line that always prints the plans, and leave the line in the traverse loop.

See here for how we do it in svcat describe plan

@jboyd01 jboyd01 force-pushed the double-plans-from-describe branch from 8b5c41c to 043c282 Compare May 10, 2018 17:29
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 10, 2018
@jboyd01 jboyd01 changed the title don't list plans twice (svcat describe plan --traverse) don't list plans twice (svcat describe class --traverse) May 10, 2018
@jboyd01
Copy link
Contributor Author

jboyd01 commented May 10, 2018

Good point @jberkhahn. I've moved the printing of plans down to the traverse section before the broker details (I think order is best to keep the plan before broker).

example output now:

$ svcat describe class dh-hello-world-apb 
  Name:          dh-hello-world-apb                   
  Description:   deploys hello-world web application  
  UUID:          9f7da06f179b895a8ee5f9a3ce4af7ef     
  Status:        Active                               
  Tags:                                               
  Broker:        ansible-service-broker               

$ svcat describe class dh-hello-world-apb  --traverse
  Name:          dh-hello-world-apb                   
  Description:   deploys hello-world web application  
  UUID:          9f7da06f179b895a8ee5f9a3ce4af7ef     
  Status:        Active                               
  Tags:                                               
  Broker:        ansible-service-broker               

Plans:
   NAME              DESCRIPTION            
+---------+--------------------------------+
  default   A sample APB which deploys      
            Hello World                     

Broker:
  Name:     ansible-service-broker  
  Status:   Ready   

Copy link
Contributor

@carolynvs carolynvs left a comment

Choose a reason for hiding this comment

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

Nice catch!

I would strongly prefer that either a) we get rid of traverse altogether since I'm not sure it provides value or b) always show plans when describing a class and don't double show them when --traverse is specified.

The reason why plans are displayed by default is that it's the more common use case for that command and I wanted to show useful information by default, instead of hiding it behind a flag.

What do you think?

@jboyd01
Copy link
Contributor Author

jboyd01 commented May 15, 2018

The class and plans do go hand in hand.... I can see reason to include it. Yet, the command says show me the class, not the class and plans. And wouldn't the describe plan duplicate the plan details we include in the describe class? Not that there is anything wrong with multiple ways to get the same info...

I'd kind of like a svcat show-me-everything ordered by broker -> class -> plans, but that is beside the point. I guess I was thinking I don't see much use of traverse.

@jberkhahn
Copy link
Contributor

Now that I think about it, I think it would make the most sense to remove the traverse flag entirely and just show the plan names by default. If the user wants more info on plan foo, they can get plans foo.

Also, how are you even supposed to know the traverse flag exists? It doesn't show up when i svcat get classes -h

@jboyd01
Copy link
Contributor Author

jboyd01 commented May 15, 2018

ok, I'm on it, will remove traverse

@carolynvs
Copy link
Contributor

And wouldn't the describe plan duplicate the plan details we include in the describe class

describe class just lists the names of the plans associated with the class while describe plan shows additional details about a specific plan, such as parameter schema and eventually defaults and other stuff.

Yet, the command says show me the class, not the class and plans

kubectl is a generic cli that doesn't have any domain knowledge, so it does things like "I will only show you the one thing you asked for". It has been my goal with svcat to show you things that are useful and most commonly desired without forcing extra calls or flags, which is why listing the plan names for a class occurs by default. I want to avoid being too pendanic with svcat, so that in order to do a common task someone doesn't have to chain multiple commands together.

@carolynvs
Copy link
Contributor

Sorry, --traverse is a wart that I brought along with the cli and should have taken care of it before it was merged upstream! 😊

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 15, 2018
Copy link
Contributor

@carolynvs carolynvs left a comment

Choose a reason for hiding this comment

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

LGTM! 💯

@jpeeler jpeeler added the LGTM2 label May 15, 2018
@jpeeler jpeeler merged commit cd1c2fd into kubernetes-retired:master May 15, 2018
@jboyd01 jboyd01 deleted the double-plans-from-describe branch May 21, 2018 19:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. LGTM1 LGTM2 size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants