search

fgeller / kt

Kafka command line tool that likes JSON
MIT License
950 stars 100 forks source link

add certificate options for all commands #86

Closed rwaweber closed 5 years ago

rwaweber commented 5 years ago

Despite the branch name these changes would add certificate support to all available commands.

Happy to restructure them if this could be improved/simplified!

fgeller commented 5 years ago

Hey, thanks for the PR, give me a few days to get back into Wifi country and I'll take a look then 🙏

On 10. Oct 2018, at 20:27, Will Weber notifications@github.com wrote:

Despite the branch name these changes would add certificate support to all available commands.

Happy to restructure them if this could be improved/simplified!

You can view, comment on, or merge this pull request online at:

https://github.com/fgeller/kt/pull/86

Commit Summary

add certificate options to all commands File Changes

M README.md (7) M common.go (27) M consume.go (22) M group.go (22) M produce.go (22) M topic.go (22) Patch Links:

https://github.com/fgeller/kt/pull/86.patch https://github.com/fgeller/kt/pull/86.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

rwaweber commented 5 years ago

Great suggestions all around! I'm not sure if I totally understood the third section but I tossed around two approaches:

  1. Clean up the invocation of the setupCerts function in each of the command's, set the variables for the sarama config closer together(what I went with).
  2. Passing in a sarama config object to the setupCerts function, or a function with 80% of the same body, that would also set the sarama config variables and return the, now modified, sarama config. I wasn't too much of a fan of this approach as I feel like it overcomplicated this function a bit and felt that most of the interaction with the sarama config should be handled with the rest of the functions that deal with it.

Totally down to revise my approach here as well!

In terms of testing, I've been using this branch at work without issue! Unfortunately, a tls-enabled kafka cluster isn't trivial to setup for CI/integration test purposes, at least the last time I took a look at it.

fgeller commented 5 years ago

@rwaweber awesome, thanks for that! i'll pull this in as is, and will apply a change so that we provide feedback if only one or two of ca, cert and key are provided. i'd ping you on the PR if you're interested.

re testing: depending on time, i'll try my hands at changing the system test - but if you're using this as is I don't see any reason not to pull this into master (ie, these changes should not interfere if someone isn't supplying the TLS specific args)