-
Notifications
You must be signed in to change notification settings - Fork 19
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: datasets
/ key-value-stores
commands
#685
base: master
Are you sure you want to change the base?
Conversation
datasets create
/ key-value-stores create
datasets create
/ key-value-stores create
& rm
counterparts
datasets create
/ key-value-stores create
& rm
counterpartsdatasets
/ key-value-stores
commands
b217317
to
3761d5e
Compare
3761d5e
to
db1473c
Compare
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)
create cmds
rm cmds (ignore the missing space, it has been fixed after taking the screenshot)
rename cmds