Skip to content

CHEF-3237: Incorporated the comments on PR 624#767

Closed
ringods wants to merge 4 commits into
masterfrom
unknown repository
Closed

CHEF-3237: Incorporated the comments on PR 624#767
ringods wants to merge 4 commits into
masterfrom
unknown repository

Conversation

@ringods

@ringods ringods commented May 17, 2013

Copy link
Copy Markdown

I rebased Igor's changes and incorporated Bryan's review comments on pull request 624.

iafonov and others added 2 commits May 17, 2013 10:22
…ot set

* Do not add `~/Library/LaunchAgents` if HOME is not set
* It is safe to assume that if HOME is not set ~ will not expand - see rb_home_dir function in MRI ruby
Comment thread lib/chef/provider/service/macosx.rb Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ideally we would use "#{ENV['HOME']}/Library/LaunchAgents" here. This should work fine, because elsewhere we use File.expand_path which will do the same thing, but that's a bit removed from this code and it took a minute to confirm we did so. If that was ever changed such that we were interpreting paths with a shell, not all shells expand tilde the same way.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Bryan,

On 20 May 2013 21:26, Bryan McLellan notifications@github.com wrote:

Ideally we would use "#{ENV['HOME']}/Library/LaunchAgents" here. This
should work fine, because elsewhere we use File.expand_path which will do
the same thing, but that's a bit removed from this code and it took a
minute to confirm we did so. If that was ever changed such that we were
interpreting paths with a shell, not all shells expand tilde the same way.

Pull request is updated with your suggestion.

Ringo

@btm

btm commented May 31, 2013

Copy link
Copy Markdown
Contributor

Merged to master.

@btm btm closed this May 31, 2013
@chef chef locked and limited conversation to collaborators Nov 16, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants