feat(kafka): add interactive prompt for kafka use#510
Conversation
rkpattnaik780
changed the title
craicoverflow
left a comment
craicoverflow
left a comment
There was a problem hiding this comment.
One change I think we should definitely make is increasing the limit on the list Kafkas, the rest are nits.
pkg/cmd/kafka/use/use.go
Outdated
| func runUse(opts *Options) error { | ||
|
|
||
| if opts.interactive { | ||
| // run the create command interactively |
There was a problem hiding this comment.
| // run the create command interactively | |
| // run the use command interactively |
pkg/cmd/kafka/use/use.go
Outdated
| return err | ||
| } | ||
|
|
||
| opts.name = *selectedKafka.Name |
There was a problem hiding this comment.
Minor nit: I like to use the getter methods when they are available, it looks cleaner than pointers and removes the unlikely even of a nil pointer error.
| opts.name = *selectedKafka.Name | |
| opts.name = selectedKafka.GetName() |
pkg/kafka/kafka.go
Outdated
| @@ -17,11 +18,11 @@ func InteractiveSelect(connection connection.Connection, logger logging.Logger) | |||
| response, _, apiErr := api.Kafka().ListKafkas(context.Background()).Execute() | |||
There was a problem hiding this comment.
I think we should set the limit to something higher than the default limit, which is 100 I believe. In the event that a user is in an organisation which has > 100 instances, not all will show up to select from.
There was a problem hiding this comment.
How much should we set it exactly?
| one = 'unable to list Kafka instances' | ||
|
|
||
| [kafka.common.input.instanceName.message] | ||
| description = 'title for the Partitions input' |
There was a problem hiding this comment.
Wrong description (but you can remove the description as the title is already self describing.
There was a problem hiding this comment.
In fact in most cases, you need not add the description, unless it is unclear from the message itself what its purpose is.
rkpattnaik780
force-pushed
the
kafka_use_prompt
branch
from
ebbd754 to
744b689
Compare
craicoverflow
left a comment
craicoverflow
left a comment
There was a problem hiding this comment.
One change needed.
| if len(args) > 0 { | ||
| opts.name = args[0] | ||
| } else if opts.id == "" { | ||
| return errors.New(localizer.MustLocalizeFromID("kafka.common.error.idFlagRequired")) | ||
| opts.interactive = true | ||
| } |
There was a problem hiding this comment.
There should be an error thrown when TTY is unavailable also.
craicoverflow
left a comment
craicoverflow
left a comment
There was a problem hiding this comment.
Great work! I have verified this in a non-TTY simulation by running the following:
❯ (setsid ./rhoas kafka use) </dev/null |& cat
Error: --id or name required when not running interactively| } | ||
|
|
||
| if response.Size == 0 { | ||
| logger.Info("No Kafka instances") |
There was a problem hiding this comment.
Thank you for localizing things as you go along :)
rkpattnaik780
force-pushed
the
kafka_use_prompt
branch
from
f47d7a8 to
69000b6
Compare
2 participants
Description
rhoas kafka useshould prompt a list of available kafka instances.fixes #496
Verification Steps
go run ./cmd/rhoas kafka useType of change
Checklist
- [ ] Documentation added for the feature