-
Notifications
You must be signed in to change notification settings - Fork 38
feat: datasets / key-value-stores commands
#685
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
Conversation
datasets create / key-value-stores createdatasets create / key-value-stores create & rm counterparts
datasets create / key-value-stores create & rm counterpartsdatasets / key-value-stores commands
3761d5e to
db1473c
Compare
B4nan
left a comment
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.
looking good, but derails from the specs in the create --name i believe
| static override description = 'Creates a new Dataset on your account'; | ||
|
|
||
| static override args = { | ||
| datasetName: Args.string({ |
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.
why not call this just name? we are in the datasets namespace, so it should be clear, no?
looking at the specs, there actually is just --name instead of --dataset-name. or this is positianal argument? again, specs mentions --name instead
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 need to poke @netmilk about this. We've had some other commands before that took --name as a flag that was then changed to a positional argument.
| })(); | ||
|
|
||
| try { | ||
| await existingDataset.datasetClient.update({ name: unname ? (null as never) : newName! }); |
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.
sounds like the types in client should be updated to allow null
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.
Yeah... among many other type changes that the client needs to get 😢. I'm really hoping we can stop manually maintaining these types and build from openAPI but even that spec misses fields -w-
| static override hiddenAliases = ['kvs:create']; | ||
|
|
||
| static override args = { | ||
| keyValueStoreName: Args.string({ |
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.
same here, specs say it should be a --name flag
(no strong opinion here, i don't mind either)
b1d3867 to
dd6e22f
Compare
create cmds

rm cmds (ignore the missing space, it has been fixed after taking the screenshot)

rename cmds
