Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Docs: reflect clients update (since 10.6.0) #83

Merged
merged 2 commits into from
Apr 13, 2021

Conversation

kares
Copy link
Contributor

@kares kares commented Apr 12, 2021

@kares kares requested a review from karenzone April 12, 2021 10:10
@karenzone karenzone added the documentation Improvements or additions to documentation label Apr 13, 2021
Copy link
Contributor

@karenzone karenzone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Builds cleanly and renders as expected. I left a comment inline for your consideration, but I trust your judgement on the resolution. LGTM

@@ -2,8 +2,8 @@
:plugin: kafka
:type: input
:default_codec: plain
:kafka_client: 2.4
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for updating these vars. I'm happy to seem them being used. :-)

We used version 2.5.1 in the integration doc and version 2.5 for both input and output docs. We should be consistent. Perhaps we should have your version checking in place before we commit to maintaining more granular versions (2.5.1 instead of 2.5).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea here is we need to have the precise version somewhere (to be able to know bug fixes etc).

For the input/output specific documentation it seemed okay to only have the minor version compatibility (the Kafka docs linked only uses minor version paths such as /25/...) but I am fine changing that if you prefer so.

I do have the version checking PR as a follow-up on this one.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with merging as is, and waiting for your version checking PR to go more granular on version info in input and output. I have more changes I need to make anyway, so I can make the fixes after version checking is merged. Thanks!

@kares kares merged commit 383abd3 into logstash-plugins:master Apr 13, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants