-
Notifications
You must be signed in to change notification settings - Fork 90
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
Updated construct_api logic to parse host value #198
Conversation
lib/fluent/plugin/out_splunk_hec.rb
Outdated
host = @hec_host.delete_prefix("/").delete_suffix("/").split("/", 2) | ||
host_suffix = host[1].nil? ? "" : "/#{host[1]}" | ||
URI("https://#{host[0]}:#{@hec_port}#{host_suffix}/services/collector") |
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.
it can only support one path, right? can it be updated so that it can support any number of it?
like https://mydomain.com/splunk/hec
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.
maybe a better approach would be adding another parameter. full_url
.
use full_url
if provided. if not build one using host
, port
, and protocol
.
@chaitanyaphalak any thoughts?
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.
it can only support one path, right? can it be updated so that it can support any number of it? like
https://mydomain.com/splunk/hec
It will support any number of path as I am splitting host in 2 half only.
For mydomain.com/splunk/hec
, it will split into mydomain.com
and splunk/hec
and will be join them with port value
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.
I like rock's idea
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.
@harshit-splunk i see. so it works.
hmm still, this way of parsing is not intuitive for the user.
instead of full_url
we can add url_path
parameter.
default would be ""
but user can set it to /splunk/hec
or anything.
However, having a full_url
parameter is something that will benefit usability in the future. we can use it in the helm chart to reduce number of arguments that they need to pass.
this way, we are consistent with olly chart. (ie splunkPlatform.endpoint
)
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.
@harshit-splunk
So I would like to suggest to edit this PR with full_url
approach.
Resolving splunk/splunk-connect-for-kubernetes#665