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

Patch for Issue #301 #302

Merged
merged 2 commits into from
Sep 28, 2016
Merged

Patch for Issue #301 #302

merged 2 commits into from
Sep 28, 2016

Conversation

radamanthus
Copy link
Contributor

Download Redis installer in /opt.

#301

@radamanthus radamanthus mentioned this pull request Sep 27, 2016
command "cd /data && tar zxf redis-#{redis_version}.tar.gz && sync"
not_if { FileTest.directory?("/data/#{redis_source_directory}") }
execute "unarchive Redis installer" do
command "cd /opt && tar zxf redis-#{redis_version}.tar.gz && sync"
Copy link
Contributor

@AlexMorreale AlexMorreale Sep 27, 2016

Choose a reason for hiding this comment

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

In Chef it's generally better to use the cwd '/opt/my/dir then to do a cd /opt/my/dir && cmd

execute "unarchive Redis installer" do
  cwd "/opt"
  command "tar zxf redis-#{redis_version}.tar.gz && sync"
  ...
end

end

execute "run redis-source/make install" do
command "cd /data/redis-source && make install"
command "cd #{redis_installer_directory} && make install"
Copy link
Contributor

Choose a reason for hiding this comment

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

same as previous comment in terms of using cwd vs cd /some/where && cmd

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback! I've updated the branch.

@AlexMorreale
Copy link
Contributor

Just a general thought but this will never install again as long as you just have that redis_source_directory. Won't that lead to false positives on the not_ifs if you ever delete the redis source or that dir. Wouldn't it be better to test for another file in the system like the redis-server binaries?

@radamanthus
Copy link
Contributor Author

Completely agree on the concern for false positives. I just have to keep this PR short and sweet, that might have to be addressed in another PR.

@dvalfre
Copy link
Contributor

dvalfre commented Sep 28, 2016

Thanks @AlexMorreale, have opened #303 for further addressing the false positives.

@dvalfre dvalfre merged commit 7cb6854 into master Sep 28, 2016
@AlexMorreale
Copy link
Contributor

Awesome!!! Happy to help/contribute.

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.

3 participants