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 quotes for configuration options #37

Merged
merged 20 commits into from
Aug 28, 2023
Merged

Fix quotes for configuration options #37

merged 20 commits into from
Aug 28, 2023

Conversation

nicolasparada
Copy link
Contributor

@nicolasparada nicolasparada commented Aug 25, 2023

Fixes an issue with quoted strings in config.
They are not unquoted when reading, causing unexpected results. For example, on the http_loader you can use multiline on the yaml format, when converting it back to classic format, everything gets inlined inside double quotes, and quotes inside the template functions get escaped. This causes template execution to fail. This PR should fix it.

Important. Looks like the tests were not working as expected and were not asserting anything. The output was split by \n and then for each line was doing an assertion. But didn't check that there were no assertions...
Now they run, but noticed that the output plugins fails with an error without enough details. Could not fix it on this PR, so for the test I stopped testing the output plugin and used a builtin fluent-bit plugin file which does exactly what the output plugin in the tests does.
We will have to fix this problem.
The error was just:

fatal: morestack on g0

The output plugin error is unrelated to this PR so this should be good to merge.

https://app.asana.com/0/1205296321537082/1205365765880566

@nicolasparada nicolasparada marked this pull request as ready for review August 25, 2023 20:33
@nicolasparada nicolasparada changed the title unquote unquote config strings Aug 25, 2023
@nicolasparada
Copy link
Contributor Author

nicolasparada commented Aug 26, 2023

I need help, there is a testdata/Dockerfile which runs fluent-bit with a config and we mount a temporal file into /fluent-bit/etc/output.txt but the output plugin file is giving Permission Denied error on the file path /fluent-bit/etc/output.txt.

I don't know how to fix this since it works great locally on MacOS, but fails on CI.

Copy link
Contributor

@niedbalski niedbalski left a comment

Choose a reason for hiding this comment

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

Minor fix required. Indeed it works on mac. I will review this a bit deeper

➜ plugin (main) ✔ go test -v ./...
?   	github.com/calyptia/plugin/input	[no test files]
?   	github.com/calyptia/plugin/metric	[no test files]
?   	github.com/calyptia/plugin/metric/cmetric	[no test files]
=== RUN   TestPlugin
--- PASS: TestPlugin (101.05s)
PASS
ok  	github.com/calyptia/plugin	102.122s
=== RUN   TestGetRecord
--- PASS: TestGetRecord (0.00s)
PASS
ok  	github.com/calyptia/plugin/output	(cached)

testdata/Dockerfile Outdated Show resolved Hide resolved
plugin_test.go Show resolved Hide resolved
Signed-off-by: Jorge Niedbalski <j@calyptia.com>
@niedbalski niedbalski changed the title unquote config strings Fix quotes for configuration options Aug 28, 2023
Copy link
Contributor

@niedbalski niedbalski left a comment

Choose a reason for hiding this comment

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

LGTM

@niedbalski niedbalski merged commit da513de into main Aug 28, 2023
10 checks passed
@niedbalski niedbalski deleted the unquote branch August 28, 2023 09:25
@@ -37,6 +37,17 @@ func testPlugin(t *testing.T, pool *dockertest.Pool) {
f, err := os.Create(filepath.Join(t.TempDir(), "output.txt"))
assert.NoError(t, err)

defer func() {
err := os.RemoveAll(f.Name())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not required since I was using t.TempDir() and that entire directory gets removed once the test finishes.

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.

2 participants