Skip to content
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

fix: handling optional bool parameters for Splunk ClusterOutput #428

Merged
merged 2 commits into from
Oct 25, 2022

Conversation

ITler
Copy link
Contributor

@ITler ITler commented Oct 20, 2022

What this PR does / why we need it:

This PR fixes nil pointer exceptions while rendering fluentbit config, when Splunk ClusterOutput does not define boolean attributes.
It was fixed by intention in a way to explicitly show Off values when the respective boolean attributes are explicitly defined in the ClusterOutput definition, despite the fact that Off might be the default value when not provided.

It is confirmed working, locally, now.

Which issue(s) this PR fixes:

Fixes #
no issue raised, yet, as the Splunk output was contributed by 'us' and there was no fluent-operator release for that, yet.

Does this PR introduced a user-facing change?

None

Additional documentation, usage docs, etc.:


Signed-off-by: ITler <ITler@users.noreply.github.com>
@wenchajun
Copy link
Member

@momoXD007
Copy link
Contributor

@wenchajun You mean to say, we should code it like this:

	if o.HTTPDebugBadRequest != nil {
 		if *o.HTTPDebugBadRequest {
 			kvs.Insert("http_debug_bad_request", "On")
 		}
        }

I think this solution will confuse people that set this option to false. If someone sets it to false they expect to see "Off" in the generated fluent-bit-config. When we use the above code: setting http_debug_bad_request to false will have no visible effect on the generated fluent-bit-config. I mean it will work as the implicit default is "Off" but it will be confusing for most people. Simply because they do not see the "Off" in the config.

@ITler
Copy link
Contributor Author

ITler commented Oct 24, 2022

We can code it as you would prefer wenchajun - just let us know. We just want to get the PR merged and, more important for us, would like to get a new release out of it ASAP.

@wenchajun
Copy link
Member

First, fluentbit is written in c. In the configuration file, on and true are equivalent. So we write the code like this:

if o.HTTPDebugBadRequest != nil {
		kvs.Insert("http_debug_bad_request", fmt.Sprint(*o.HTTPDebugBadRequest))
	}

Second, because of the first point, so you have written in that cr is true, the same in the generated configuration is filled with true easier to understand. Because for the user, it is the cr that is edited and not the secret. third, in other apis, we use the above form, so I think it is better to unify.
We can discuss more about the way the code is written and choose a relatively suitable method to complete it.

@ITler
Copy link
Contributor Author

ITler commented Oct 24, 2022

Ha! We were not aware of the On|true ... yeah, than fully agree. We will change it, no need to discuss further 👍

@wenchajun
Copy link
Member

Seems that DCO failed, please commit with a signature

git commit -s --amend
git push -f

Signed-off-by: ITler <ITler@users.noreply.github.com>
@ITler
Copy link
Contributor Author

ITler commented Oct 24, 2022

Seems that DCO failed, please commit with a signature

git commit -s --amend
git push -f

fixed... sorry, I am not used to that.

@ITler
Copy link
Contributor Author

ITler commented Oct 24, 2022

Would you be able to create a new release of fluent-operator then?

@benjaminhuo benjaminhuo merged commit 1c77ff3 into fluent:master Oct 25, 2022
@benjaminhuo
Copy link
Member

Would you be able to create a new release of fluent-operator then?

Sure, @wenchajun will release v1.6 soon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants