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

New parameter for home directory #368

Merged
merged 13 commits into from
Jul 20, 2022
Merged

New parameter for home directory #368

merged 13 commits into from
Jul 20, 2022

Conversation

joernott
Copy link
Contributor

@joernott joernott commented Jun 5, 2018

This solves #367

@ninaspitfire
Copy link
Contributor

Thanks! Could you please add a section to the README so that people know it's there and how to use it without reading the code?

It will also need a test like this one before we can merge. I can work on the test next week if you don't get to it before then.

Cheers.

@joernott
Copy link
Contributor Author

joernott commented Jun 6, 2018

Adding to the readme is rather trivial. Writing the test is quite a challenge, as this parameter is not used in the class directly but in the sub classes.

When running tests on our test platform, I encounbtered another issue with floating classes (trying to install the package before the yum repo was available), so I switched from "include" to "contain".

After that, I will try to see, what my test runs write into the logs, so I can write an appropriate test.

@fatmcgav
Copy link

@joernott With regards to writing an acceptance test, it should be possible to use the user server-spec resource:

have_home_directory
In order to test a user have a given home directory, you should use have_home_directory matcher.

describe user('root') do
  it { should have_home_directory '/root' }
end

@fatmcgav fatmcgav added the enhancement New feature or request label Jun 11, 2018
@ninaspitfire
Copy link
Contributor

Is this patch intended to be used with Logstash installations that are provided by other means (ie. not by this module)? I guess my question is: "how is Logstash getting into a custom location in the first place?".

If I read this correctly, setting the $home_dir won't actually change were the module installs Logstash, since that is defined by the deb/rpm packages.

@joernott
Copy link
Contributor Author

I am aware of that. Currently, we package our own logstash (and the rest of the stack to achieve compliance to our folder structure rules. We have an ugly wrapper around the puppet-logstash module which creates symlinks and has dependencies on resources inside the puppet-logstash module to work around puppet-logstash using a hard coded file path to determine file locations which are only valid if you use the original .deb|.rpm packages. Even if you unpack the official tar.gz, this module won't work. The puppet-elasticsearch module has a variable $home_dir which they use instead of hard coded paths for that reason. So this PR just introduces that feature.

@vox-pupuli-tasks
Copy link

Dear @joernott, thanks for the PR!

This is Vox Pupuli Tasks, your friendly Vox Pupuli Github Bot. I noticed that your pull request has CI failures. Can you please have a look at the failing CI jobs?
If you need any help, you can reach out to us on our IRC channel voxpupuli on Freenode or our Slack channel voxpupuli at slack.puppet.com.
You can find my sourcecode at voxpupuli/vox-pupuli-tasks

@vox-pupuli-tasks
Copy link

Dear @joernott, thanks for the PR!

This is Vox Pupuli Tasks, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase?

You can find my sourcecode at voxpupuli/vox-pupuli-tasks

@joernott
Copy link
Contributor Author

Rebase done

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

The tests didn't run, but I'd expect them to fail. The declaration of $home_dir on line 161 looks invalid to me.

manifests/init.pp Outdated Show resolved Hide resolved
manifests/init.pp Outdated Show resolved Hide resolved
manifests/init.pp Outdated Show resolved Hide resolved
joernott and others added 3 commits June 27, 2022 09:52
Co-authored-by: Ewoud Kohl van Wijngaarden <[email protected]>
Co-authored-by: Ewoud Kohl van Wijngaarden <[email protected]>
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

I still think you need to remove this line:

$home_dir = '/usr/share/logstash'

remove duplicate declaration of home_dir
@joernott joernott requested a review from ekohl July 20, 2022 08:19
@ekohl ekohl merged commit 5c72041 into voxpupuli:master Jul 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request tests-fail
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants