kafka icon indicating copy to clipboard operation
kafka copied to clipboard

Fix missing newlines in env file and log4j.properties

Open hrak opened this issue 3 years ago • 6 comments

Description

This PR fixes the missing newline issue described in #189

The change in this commit broke the newlines by replacing occurrences of $/ with $INPUT_RECORD_SEPARATOR without adding a require 'English' that defines this variable.

I have added tests to verify some of the content of both the env file and the log4j.properties file.

While fixing this issue i also found out that the serverspec test suite was actually not being executed due to an invalid verifier path when using kitchen-dokken. kitchen-dokken expects the tests to be in /opt/verifier but the default path is /tmp/verifier, causing no tests to be executed.

When the tests were working again, i found out that the test suite was actually broken. The following changes were made to restore test functionality:

  • revert some changes made in this commit since shell_out! doesn't work in serverspec.
  • Fixed the env file location for Ubuntu

The best thing would probably be to convert all tests to inspec, but I'll leave that for another PR :)

Issues Resolved

#189 #190

Check List

  • [x] All tests pass. See TESTING.md for details.
  • [x] New functionality includes testing.
  • [ ] New functionality has been documented in the README if applicable.

hrak avatar May 06 '21 21:05 hrak

@hrak looks like replacing shell_out with run_command is now deprecated according to Cookstyle.

damacus avatar May 07 '21 07:05 damacus

@hrak looks like replacing shell_out with run_command is now deprecated according to Cookstyle.

I know, i described that in the comment above. But shell_out doesn't work with serverspec, that's why i reverted that change. For this to pass cookstyle we'll have to either disable the ChefModernize/ShellOutHelper cop for the tests, or rename the run_command method to something that doesn't trigger cookstyle. What do you prefer?

hrak avatar May 07 '21 07:05 hrak

Ah there's our problem. We shouldn't be using serverspec but InSpec.

damacus avatar May 07 '21 10:05 damacus

To be honest. This cookbook really needs an overhaul. I'm fairly sure this isn't going to easily pass any recent cookstyle or spec testing 🤔

damacus avatar May 07 '21 10:05 damacus

To be honest. This cookbook really needs an overhaul. I'm fairly sure this isn't going to easily pass any recent cookstyle or spec testing 🤔

Its not that bad actually. I just tested with latest chef workstation and with some minor changes we can make this build pass:

diff --git a/.rubocop.yml b/.rubocop.yml
index d88ffa9..78dda6d 100644
--- a/.rubocop.yml
+++ b/.rubocop.yml
@@ -2,3 +2,8 @@
 AllCops:
   Exclude:
     - 'Dangerfile'
+
+Chef/Deprecations/UsesRunCommandHelper:
+  Exclude:
+    - 'test/integration/helpers/serverspec/support/service_common.rb'
+    - 'test/integration/systemd/serverspec/service_spec.rb'
diff --git a/test/integration/systemd/serverspec/required_files_spec.rb b/test/integration/systemd/serverspec/required_files_spec.rb
index e39bce9..a326d77 100644
--- a/test/integration/systemd/serverspec/required_files_spec.rb
+++ b/test/integration/systemd/serverspec/required_files_spec.rb
@@ -23,7 +23,7 @@ describe 'required files for systemd init style' do
     end

     it 'is properly formatted with newlines' do
-      expect(env_file.content).to match(%r{SCALA_VERSION=.*\nKAFKA_OPTS=.*\nJMX_PORT=.*})
+      expect(env_file.content).to match(/SCALA_VERSION=.*\nKAFKA_OPTS=.*\nJMX_PORT=.*/)
     end
   end

Let me know if you want me to push these changes, then after this PR we can work on the inspec conversion. I also found that there are some unused test suites that can be removed (runit, sysv, upstart).

hrak avatar May 07 '21 11:05 hrak

So what should be the way forward here? You want me to convert the serverspec tests to inspec?

hrak avatar Oct 14 '21 14:10 hrak

closing due to age. Feel free to reopen if you'd like to continue working on this. Currently the recommended cookbook approach is to use inspec for testing and chefspec only as needed for library helpers and others that inspec may be more of a lengthy heavy test.

Stromweld avatar May 03 '24 22:05 Stromweld