Skip to content

Add parameter to manage the content of log4j2.properties #1260

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

Merged
merged 2 commits into from
Jun 27, 2025

Conversation

h-haaks
Copy link
Contributor

@h-haaks h-haaks commented Jun 27, 2025

Pull Request (PR) description

Add a parameter to manage the content of the log4j config file
Very simple solution that just replaces the entire content of the file.

This PR fixes review comments in #1234 so that we can get this merged before the next release.

This Pull Request (PR) fixes the following issues

Replace Opened PR

@h-haaks h-haaks force-pushed the new-log4j-support branch from 94664b6 to ded149d Compare June 27, 2025 15:21
@h-haaks h-haaks force-pushed the new-log4j-support branch from ded149d to 2e32949 Compare June 27, 2025 16:12
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is something really strange going on in this file...
It has a double set of on_supported_os blocks, and I don't thing any of the tests in the second block is ran at all.

@h-haaks h-haaks marked this pull request as ready for review June 27, 2025 17:06
@h-haaks h-haaks added the enhancement New feature or request label Jun 27, 2025
@h-haaks h-haaks changed the title New log4j support Add parameter to manage the content of log4j2.properties Jun 27, 2025
@h-haaks
Copy link
Contributor Author

h-haaks commented Jun 27, 2025

I tried to figure out where to add some acceptance testing for this, but I have no idea where to put them without creating a full shared examples file setup.
Decided not to do that here.

@h-haaks
Copy link
Contributor Author

h-haaks commented Jun 27, 2025

I'll fixup commits before merge.

Comment on lines +375 to +378
it {
expect(subject).to contain_file('/etc/elasticsearch/log4j2.properties').
with(ensure: 'file', content: "# Content Line 1\n# Content Line 2")
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
it {
expect(subject).to contain_file('/etc/elasticsearch/log4j2.properties').
with(ensure: 'file', content: "# Content Line 1\n# Content Line 2")
}
it do
expect(subject).to contain_file('/etc/elasticsearch/log4j2.properties').
with(ensure: 'file', content: "# Content Line 1\n# Content Line 2")
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, multiline ruby blocks should use do..end.
But the entire module is written like this and I don't want to brake that pattern in just one test.
No rubocop autocorrect either ..

@h-haaks h-haaks merged commit 9f28467 into master Jun 27, 2025
49 checks passed
@h-haaks h-haaks deleted the new-log4j-support branch June 27, 2025 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
4 participants