confluentinc / confluent-cli

Confluent Platform CLI
Other
60 stars 38 forks source link

[WIP] Wrapper around bin scripts, include Avro #91

Closed ybyzek closed 6 years ago

ybyzek commented 6 years ago

Not Avro:

confluent-cli(consolebin-wrapper) ✗: confluent produce mytopic2                 
This CLI is intended for development only, not for production
https://docs.confluent.io/current/cli/index.html

>a
>b
>c
>^C%                 

$ confluent-cli(consolebin-wrapper) ✗: confluent consume mytopic2 --from-beginning
This CLI is intended for development only, not for production
https://docs.confluent.io/current/cli/index.html

a
b
c
^CProcessed a total of 3 messages

Avro:

confluent-cli(consolebin-wrapper) ✗: confluent produce mytopic4 --value-format avro --property value.schema='{"type":"record","name":"myrecord","fields":[{"name":"f1","type":"string"}]}'
This CLI is intended for development only, not for production
https://docs.confluent.io/current/cli/index.html

newargs: mytopic4  --property value.schema={"type":"record","name":"myrecord","fields":[{"name":"f1","type":"string"}]}
{"f1": "value1"}
{"f1": "value1"}
{"f1": "value1"}
^C

$ confluent-cli(consolebin-wrapper) ✗: confluent consume mytopic4 --value-format avro --from-beginning
This CLI is intended for development only, not for production
https://docs.confluent.io/current/cli/index.html

{"f1":"value1"}
{"f1":"value1"}
{"f1":"value1"}
^CProcessed a total of 3 messages
ybyzek commented 6 years ago

@jrjain , @kkonstantine

I believe the summary of outstanding issues to resolve are of a philosophical nature:

  1. Do we want users to be able to use confluent produce/consume for non-local Kafka clusters? I strongly believe the answer is yes, and thus the code is written to accept overrides for broker-list and bootstrap-servers. I believe @kkonstantine feels the other way.

  2. Do we want users to be able to explore and use other options provided by the bin commands, which may or may not conform to future Converged CLI? I believe the answer is yes to improve greatest utility, and thus the code is written to point users in that direction. I believe @kkonstantine feels the other way. Possible compromise: remove mention of it from the usage description, but still allow those args to be passed in.

cyrusv commented 6 years ago

The code makes sense, but I'm confused why one flag is --value-format avro and the other is --property value.schema instead of having them match, e.g. value-schema=?

Also I'm confused why --value-format takes a space, and `value-schema takes an = to separate from argument? Might be overkill to support both options, but consistency is good.

https://stackoverflow.com/questions/7069682/how-to-get-arguments-with-flags-in-bash-script shows a nice idiom, but is probably overkill

ybyzek commented 6 years ago

@cyrusv , it's because this is hybrid of two CLIs: --value-format avro is in anticipation of upcoming Converged CLI and --property value.schema is what's currently available in the bin command

edenhill commented 6 years ago

Would it make sense to abstract the difference and just expose --avro | --avro-value | --avro-key and then use the proper argument based on underlying tool?

ybyzek commented 6 years ago

Would it make sense to abstract the difference and just expose --avro | --avro-value | --avro-key and then use the proper argument based on underlying tool?

@edenhill there is an argument for that, but --value-format avro was by request of @jrjain to use the Converged CLI syntax.