-
Notifications
You must be signed in to change notification settings - Fork 41
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: stencil cli for namespace and schema #97
Conversation
cmd/schema.go
Outdated
"group:core": "true", | ||
}, | ||
RunE: func(cmd *cobra.Command, args []string) error { | ||
s := term.Spin("Fetching schema list") |
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.
@ishanarya0 There is a slight change in this API for the spinner. Check guardian for reference. Also we can avoid label since this is only action and we are not changing any state.
cmd/schema.go
Outdated
Short: "list all schemas", | ||
Args: cobra.ExactArgs(1), | ||
Example: heredoc.Doc(` | ||
$ stencil schema list <schema-id> |
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.
let's replace schema-id
with namespace-id
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.
This should be a flag right. If not provided it list schemas across all namespaces.
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.
Need API support for that. Currently, it returns no schemas if namespace-id is not given.
cmd.Flags().StringVar(&host, "host", "", "stencil host address eg: localhost:8000") | ||
cmd.MarkFlagRequired("host") | ||
|
||
cmd.Flags().Int32VarP(&version, "version", "v", 0, "particular version to be deleted") |
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.
any reason why version is flag, instead of args?
namespace and schema names are args. We should ideally put version also as args.
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.
@harikrishnakanchi Version is just a filter for a given schema. the major entity is a schema, hence the flag
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 mean we can still make version as optional with args as well. If version is omitted, then it retrieves latest version data.
This command structure would be better
stencil schema get <namespace-id> <schema-id> <version> -m
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.
More than one org in cli is bad practice. I would recommend following.
stencil schema get <schema-id> -n <namespace> -v <version> -m -o <output format>
This takes schema-id as org as we are getting schema everything else is a modifier.
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.
Yes, this looks good. Changing every cmd to take a max of one arg.
cmd/schema.go
Outdated
Short: "version(s) of all schemas", | ||
Args: cobra.ExactArgs(2), | ||
Example: heredoc.Doc(` | ||
$ stencil schema list <namespace-id> <schema-id> |
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.
example should be corrected to stencil schema version <namespace-id> <schema-id>
Changes -
|
cmd/schema.go
Outdated
cmd.MarkFlagRequired("host") | ||
|
||
cmd.Flags().Int32VarP(&version, "version", "v", 0, "version of the schema") | ||
cmd.MarkFlagRequired("host") |
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.
redundant. host flag already marked as required.
cmd/schema.go
Outdated
cmd.MarkFlagRequired("host") | ||
|
||
cmd.Flags().BoolVarP(&metadata, "metadata", "m", false, "set this flag to get metadata") | ||
cmd.MarkFlagRequired("host") |
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.
redundant. Host flag already marked as required. this can be removed.
Proposed features
A. Namespace
B. Schema