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

[CLI Improvements] Support all resource #2304

Closed
Tracked by #2182
schoren opened this issue Apr 3, 2023 · 12 comments · Fixed by #2501
Closed
Tracked by #2182

[CLI Improvements] Support all resource #2304

schoren opened this issue Apr 3, 2023 · 12 comments · Fixed by #2501
Assignees

Comments

@schoren
Copy link
Collaborator

schoren commented Apr 3, 2023

We want to support the all resource for verbs. It acts as a pseudo resources that actually wraps all available resources.

For example, if you have configured a DataStore, a Demo, and a test, you could export them all to a yaml file like this:

tracetest list all -o yaml > dump.yaml

you could import that dump with the following command:

tracetest apply all -f dump.yaml

Verbs supported:

  • list
  • apply

get and delete always require an ID, and that ID only makes sense when refering to a specific resource, so it doesn't really apply for those.

@schoren schoren mentioned this issue Apr 3, 2023
19 tasks
@schoren
Copy link
Collaborator Author

schoren commented Apr 3, 2023

There is an inconsistency here. Not all resources support the list verb. For example, you cannot list Configs since there is only one. One solution would be to make all ignore those constraints and somehow force the resource to be part of the list. This is a bit inconsistent. We could get rid of the inconcistency by "faking" the list verb support for such resources, but it still feels hacky.

Another solution would be to merge the get and list verbs, as we disucssed at some point. With this approach, we would replace this with get all, that would work for single and multiple resources (Config and Tests, for example). To get a single row you'd do tracetest get test --id someid and skipping the --id flag would just get the list. We discussed this approach, refering to how kubectl does the get verb.

@kdhamric @olha23 thoughts?

@olha23
Copy link

olha23 commented Apr 5, 2023

@schoren not sure that i am the best person to suggest here but i think that both solutions have their pros and cons, so it ultimately depends on specific use case and preferences. If the goal is to simplify the CLI and merge the list and get verbs, then introducing a new "get all" verb could work. This would make it consistent for all resources and avoid confusion between the list and get verbs.

@kdhamric
Copy link
Collaborator

kdhamric commented Apr 5, 2023

The issue currently with the config resource is that it is hard to understand how to 'get' it.

If I do:
tracetest get config
ERROR id of the resource to get must be specified

So... I need to know the id. So I would naturally do:
tracetest list config
ERROR failed to list for type: config {"error": "the specified resource type doesn't support the action"}

I am then 'stuck' not knowing the id for a config (it is current, but how can I know this as a new user)

Solution:
I think we should support list. By default, it shows a nice, column based output with key info... particularly the id

@schoren
Copy link
Collaborator Author

schoren commented Apr 5, 2023

@kdhamric we defined that "unique" resources like Config do not require the --id, as defined here.

Does that make any difference in your thoughts?

@kdhamric
Copy link
Collaborator

kdhamric commented Apr 5, 2023

That is good, but I am getting more comfortable thinking list would make sense, even on resources that can only have 'one'. A couple reasons I think this:

  • trying to keep all commands working for all resources. There are exceptions (run, particularly), but in general it will be nice if a 'tracetest verb resource' works across the board.
  • if I am learning what resources are (ie a new user with no idea of what there is, what things do) I may do a 'tracetest list all' to see what all the resources are.
  • enabling list on everything makes commands such as 'tracetest list all -o yaml > dump.yaml' that you reference an issue with due to config work just fine.

I may not understand the hesitation to use the list verb when there is only one item - feel free to jump in a meeting.

@schoren
Copy link
Collaborator Author

schoren commented Apr 5, 2023

There's no "hard" reason for not supporting list in unique resources. It's just my minimalist mind trying to remove "unnecessary" things. But mainly, it was the consensus from the team.

Even if adding the list for any resource, we'll still have a few (minor) inconcistencies. For example, I would still NOT require the --id for unique resources, but the list` would show the ID. It's not a huge deal, just pointing this out.

@kdhamric do you confirm we should have list for all resources?

@kdhamric
Copy link
Collaborator

kdhamric commented Apr 5, 2023

I would tend to definitely allow 'tracetest get config --id current' as that is how people will have been trained to get an item. I am more neutral on whether to make 'tracetest get config' work.

We also need to discuss datastores, as I lot of this discussion applies to them and we may want to reconsider.

@mathnogueira
Copy link
Member

I like @olha23's idea. If list has some exceptions, why not use get all instead?

@jorgeepc
Copy link
Contributor

jorgeepc commented Apr 5, 2023

I like the idea of having a list [resource] and a get [resource] --id [id] command.

I don't think we should have exceptions or differences between resources. I know Config has one single item, but it still has an id and I don't see a limitation about listing it.

@adnanrahic
Copy link
Contributor

Agree with @jorgeepc here. Having identical behavior across all resources is what I'm voting for.

@kdhamric
Copy link
Collaborator

kdhamric commented Apr 6, 2023

Make it so...

Have a list [resource] and a get [resource] --id [id] for every resource.

@xoscar xoscar changed the title support all resource [CLI Improvements] Support all resource Apr 10, 2023
@xoscar xoscar added the CLI label Apr 10, 2023
@xoscar xoscar self-assigned this May 5, 2023
@xoscar xoscar added the backend label May 5, 2023
@xoscar
Copy link
Collaborator

xoscar commented May 5, 2023

Team, currently the list verb is paginated, by default we show chunks of 20 items.
My suggestion would be to add an --all flag that should return the entire list no matter the # of results

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants