-
Notifications
You must be signed in to change notification settings - Fork 1.7k
out_kinesis_firehose: introduce port parameter #10660
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
base: master
Are you sure you want to change the base?
Conversation
example config:
There isn't any interesting log output, but tcpdump proves it's working:
|
4193553
to
0426c9d
Compare
Approving the unit tests and tagging @cosmo0920 to take a look |
@runderwo we will also need DCO / signed commits (https://github.com/apps/dco) |
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.
Basically, the change should be valid but I'm wondering that is there any reason to be removed the validation of port range? Isn't it worth for the validation of the port range?
Plus, we need to add Signed-off line in this commit: 4f35d08 |
WalkthroughThe changes update the port handling logic for both the Kinesis Firehose and Kinesis Streams output plugins. Port assignment is now dynamic, using the port specified in the output instance configuration or defaulting to 443 if unspecified. Structs are updated to use Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant OutputInstance
participant PluginContext
User->>OutputInstance: Configure host and port
OutputInstance->>PluginContext: Initialize with host and port
PluginContext->>PluginContext: If port == 0, set to default (443)
PluginContext->>PluginContext: If port valid (1-65535), assign
PluginContext->>PluginContext: Else, log error and abort
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Oh, we need to add some prefixes into commit messages like:
out_kinesis_firehose: [And the descriptions of the change]
This mirrors the same for Kinesis Streams in fluent#9317 Signed-off-by: Ryan Underwood <[email protected]>
This mirrors the same for Kinesis Streams in #9317
The port parameter is migrated to uint16_t for correctness.
Enter
[N/A]
in the box, if an item is not applicable to your change.Testing
Before we can approve your change; please submit the following in a comment:
If this is a change to packaging of containers or native binaries then please confirm it works for all targets.
ok-package-test
label to test for all targets (requires maintainer to do).Documentation
Backporting
Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.
Summary by CodeRabbit
Bug Fixes
Refactor