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

notify "immediately" causes notified resource to run too soon #22

Merged
merged 1 commit into from
Feb 13, 2014

Conversation

JeanMertz
Copy link
Contributor

I noticed that a "notify immediately" action on this resource causes the other resource to run before file permissions/ownership are properly set by the s3_file resource.

Example:

s3_file File.join('/usr/lib/jvm/java-6-openjdk-amd64/', jai_file) do
  notifies              :run, "execute[install #{jai}]", :immediately
  remote_path           jai_file
  bucket                BUCKET
  mode                  '0755'
  aws_access_key_id     ID
  aws_secret_access_key KEY
end

execute "install #{jai}" do
  action  :nothing
  cwd     '/usr/lib/jvm/java-6-openjdk-amd64/'
  command "./#{jai_file}"
end

This will cause:

[2014-02-09T10:32:05+00:00] ERROR: execute[install jai_imageio-1_1-lib]
(myapp::app line 37) had an error: Errno::EACCES: Permission denied - ./jai_imageio-1_1-lib-linux-amd64-jdk.bin

When removing :immediately (delaying the action), it runs normally.

I haven't found the issue yet in this cookbook, though I presume it has something to do with calling the file resource.

What I did notice while looking at this resource, the file resource is only called if a file is freshly downloaded. I would expect that resource the run every time, since it involves file ownership, which could change even if the file is present. Also the file resource is capable of figuring out when to run and when not to, so it's safe to run every time this resource is called.

@JeanMertz
Copy link
Contributor Author

I added a PR which fixes this issue. It also has some general resource improvements.

@JeanMertz
Copy link
Contributor Author

@adamsb6 any chance of getting this merged with a version update? Thanks!

@adamsb6
Copy link
Owner

adamsb6 commented Feb 13, 2014

Thanks for following up! I like the additional functionality, but it looks like you also stripped the code that makes leading slashes optional.

Some tools treat S3 keys as if they were part of a root filesystem, while others follow the spec from AWS and do not allow for a leading slash. I chose to accomodate both styles, since there's little to no chance that doing so could hurt a user, while enforcing one or the other could lead to wasting a user's time.

Can you add that code back in your branch? I'll merge the pull request then, rev versions, and push the new version out to the community site after I've tested it in my environment.

@JeanMertz
Copy link
Contributor Author

@adamsb6 the leading slash is still optional.

See:

remote_path = ::File.join('', new_resource.remote_path)

Here's an example:

File.join('', 'my/path') # => "/my/path"
File.join('', '/my/path') # => "/my/path"

@adamsb6
Copy link
Owner

adamsb6 commented Feb 13, 2014

Ah, I see. I didn't expect that to be the behavior. I'll merge, then. Thanks again!

adamsb6 added a commit that referenced this pull request Feb 13, 2014
notify "immediately" causes notified resource to run too soon
@adamsb6 adamsb6 merged commit eaf2a95 into adamsb6:master Feb 13, 2014
@JeanMertz
Copy link
Contributor Author

Awesome, thanks! Looking forward to the new version on the community site.

@JeanMertz JeanMertz deleted the resource_improvements branch February 14, 2014 07:11
adamsb6 added a commit that referenced this pull request Feb 14, 2014
New version with fix for notify immediately issue,
#22.
@adamsb6
Copy link
Owner

adamsb6 commented Feb 14, 2014

These changes caused no problems in my environments, I've uploaded them to the community site.

@JeanMertz
Copy link
Contributor Author

👍 Thank you.

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.

2 participants