Local file copy resource?


#1

Looking at CHEF 1716 and CHEF-3676, I’m trying to figure out the best
way to implement local file copies.

My thoughts:

  • The File resource is very simple, and that is a good thing, don’t
    want to over load it doing too many things
  • CookbookFile is specific to installing files from a cookbook, but is
    almost identical to a local file copy. Adding a “local” option as for
    1716 doesn’t make sense, simply based on the naming

Here’s a commit that adds the local option and would close 1716:

Here’s Dan DeLeo’s comments:

cookbook_file feels a bit wrong, since we’ve scoped it to have the
functionality of pulling down files from the files/ section of a
cookbook.

Putting it directly in the file resource also seems wrong, since it
doesn’t currently have a concept of a source where the file comes
from.

This leaves two options I can think of:

  1. Add a local option to remote_file. This seems weird, since it’s not
    really remote, but it fits with the idea of pulling a file from one
    place and putting it somewhere on the local system, just in this case
    that somewhere happens to be a file system available to the local box.

  2. Add a local_file resource and provider. This is the best conceptual
    fit, and it shouldn’t be too bad to share functionality from other
    resources and providers via inheritance and composition. An argument
    against is that you’re creating “resource sprawl”.


#2

On Dec 8, 2012, at 3:41 PM, Jesse Campbell wrote:

Looking at CHEF 1716 and CHEF-3676, I’m trying to figure out the best
way to implement local file copies.

My thoughts:

  • The File resource is very simple, and that is a good thing, don’t
    want to over load it doing too many things
  • CookbookFile is specific to installing files from a cookbook, but is
    almost identical to a local file copy. Adding a “local” option as for
    1716 doesn’t make sense, simply based on the naming

Here’s a commit that adds the local option and would close 1716:
https://github.com/jcam/chef/commit/f0df80d0aac1cced64dc3d6d8cd064968dac473b

Here’s Dan DeLeo’s comments:

cookbook_file feels a bit wrong, since we’ve scoped it to have the
functionality of pulling down files from the files/ section of a
cookbook.

Putting it directly in the file resource also seems wrong, since it
doesn’t currently have a concept of a source where the file comes
from.

This leaves two options I can think of:

  1. Add a local option to remote_file. This seems weird, since it’s not
    really remote, but it fits with the idea of pulling a file from one
    place and putting it somewhere on the local system, just in this case
    that somewhere happens to be a file system available to the local box.

  2. Add a local_file resource and provider. This is the best conceptual
    fit, and it shouldn’t be too bad to share functionality from other
    resources and providers via inheritance and composition. An argument
    against is that you’re creating “resource sprawl”.

How about 3. augment file so that #content can be a block returning a Ruby file object (or string) so you could do

file “foo” do
owner "whatever"
content { ::File.new(“other”) }
end

–Noah


#3

I think I’d like to see consistency with the Template resource, using
a “source” option to specify a string filename.

I’m not exactly an expert at inheritance and composition in Ruby, but
maybe someone on here is?

Could we have a simple file resource that depending on specified
options it merges in partial classes adding required functionality?
Specify a “source” option, it merges in the necessary file copy partial class
Specify a “template_source” option it merges in the necessary
templating partial class
Specify a “remote” option it merges a partial class with the ability
to fetch things remotely

On Sat, Dec 8, 2012 at 6:46 PM, Noah Kantrowitz noah@coderanger.net wrote:

On Dec 8, 2012, at 3:41 PM, Jesse Campbell wrote:

Looking at CHEF 1716 and CHEF-3676, I’m trying to figure out the best
way to implement local file copies.

My thoughts:

  • The File resource is very simple, and that is a good thing, don’t
    want to over load it doing too many things
  • CookbookFile is specific to installing files from a cookbook, but is
    almost identical to a local file copy. Adding a “local” option as for
    1716 doesn’t make sense, simply based on the naming

Here’s a commit that adds the local option and would close 1716:
https://github.com/jcam/chef/commit/f0df80d0aac1cced64dc3d6d8cd064968dac473b

Here’s Dan DeLeo’s comments:

cookbook_file feels a bit wrong, since we’ve scoped it to have the
functionality of pulling down files from the files/ section of a
cookbook.

Putting it directly in the file resource also seems wrong, since it
doesn’t currently have a concept of a source where the file comes
from.

This leaves two options I can think of:

  1. Add a local option to remote_file. This seems weird, since it’s not
    really remote, but it fits with the idea of pulling a file from one
    place and putting it somewhere on the local system, just in this case
    that somewhere happens to be a file system available to the local box.

  2. Add a local_file resource and provider. This is the best conceptual
    fit, and it shouldn’t be too bad to share functionality from other
    resources and providers via inheritance and composition. An argument
    against is that you’re creating “resource sprawl”.

How about 3. augment file so that #content can be a block returning a Ruby file object (or string) so you could do

file “foo” do
owner "whatever"
content { ::File.new(“other”) }
end

–Noah


#4

On Saturday, December 8, 2012 at 4:09 PM, Jesse Campbell wrote:

I think I’d like to see consistency with the Template resource, using
a “source” option to specify a string filename.

I’m not exactly an expert at inheritance and composition in Ruby, but
maybe someone on here is?

Both are used in the existing source code. For example, the thing that sets all the mode and owner bits is a FileAccessControl object. Adding functionality by having classes that implement bits of it in this style is composition.

Contrast this with how the service providers implement bits of functionality directly in the class, and then more-specific service providers reuse that functionality by inheriting (e.g., the class SpecificServiceProvider < GenericServiceProvider bit). Adding modules with include also counts as inheritance.

Here’s a recent blog post comparing composition to piping Unix commands together: http://blog.codeclimate.com/blog/2012/11/28/your-objects-the-unix-way/


Daniel DeLeo


#5

On Sat, Dec 8, 2012 at 6:41 PM, Jesse Campbell hikeit@gmail.com wrote:

  • The File resource is very simple, and that is a good thing, don’t
    want to over load it doing too many things

You’re managing files, so you should be able to use the File resource.

There was discussion a few years ago about moving remote_file into file [1]
for that reason, although it never happened.

IMHO, we correctly did not repeat this pattern of creating multiple
resources based on location elsewhere after the initial implementations.
For example, we don’t need separate mount resources for local and remote
filesystems.

Adding a :to attribute to the File resource, like we have in the Link
resource and adding move/copy actions to it makes sense to me. I don’t
think it would clutter the file resource because we’re adding separate
actions, which is where the bulk of the change would be.

Cookbook File is not what you’re managing, so we shouldn’t be in that
resource. If there’s code to reuse we could refactor such, but I don’t
expect that to be ideal.

A resource should be a ‘what,’ that is, what we are abstracting. The
details should be in the action and attributes and where appropriate we
should be intuitive about it, such as seeing that a source attribute is a
URL, but we also need to be careful to not overload an attribute such that
we have to parse it for different pieces of data.

Neckbeard in spirit,
Bryan

[1] http://tickets.opscode.com/browse/CHEF-651


#6

On Sat, Dec 8, 2012 at 6:46 PM, Noah Kantrowitz noah@coderanger.net wrote:

file “foo” do
owner "whatever"
content { ::File.new(“other”) }
end

That’s too complex. “cp foo bar” is easy, we shouldn’t have to get into
ruby do accomplish that, nor fall back on the execute resource for
something this common and simple.

Thoughts on idempotency?

Copy could check checksums and copy if they differ or perms differ, sort of
like cookbook_file.
Move could be only if the source file (path) exists.

Bryan


#7

I’m taking an initial stab at combining cookbook_file and file.

Before I go too much further, do any of you know how idempotency is
working with cookbook_file?

I see this in the create:
if file_cache_location && content_stale?

content_stale looks like this:
def content_stale?
( ! ::File.exist?(@new_resource.path)) || ( ! compare_content)
end

compare_content looks like this:
# Compare the content of a file. Returns true if they are the
same, false if they are not.
def compare_content
checksum(@current_resource.path) == new_resource_content_checksum
end

new_resource_content_checksum looks like this:
def new_resource_content_checksum
@new_resource.content && Digest::SHA2.hexdigest(@new_resource.content)
end

and new_resource.content appears not to be set for cookbook_file.

What am I missing?

On Sat, Dec 8, 2012 at 10:03 PM, Bryan McLellan btm@loftninjas.org wrote:

On Sat, Dec 8, 2012 at 6:46 PM, Noah Kantrowitz noah@coderanger.net wrote:

file “foo” do
owner "whatever"
content { ::File.new(“other”) }
end

That’s too complex. “cp foo bar” is easy, we shouldn’t have to get into ruby
do accomplish that, nor fall back on the execute resource for something this
common and simple.

Thoughts on idempotency?

Copy could check checksums and copy if they differ or perms differ, sort of
like cookbook_file.
Move could be only if the source file (path) exists.

Bryan


#8

Okay… nevermind, i found it, there is a check for whether
cookbook.preferred_filename_on_disk_location returns nil. Yay for good
comments :stuck_out_tongue:

On Sun, Dec 9, 2012 at 10:44 AM, Jesse Campbell hikeit@gmail.com wrote:

I’m taking an initial stab at combining cookbook_file and file.

Before I go too much further, do any of you know how idempotency is
working with cookbook_file?

I see this in the create:
if file_cache_location && content_stale?

content_stale looks like this:
def content_stale?
( ! ::File.exist?(@new_resource.path)) || ( ! compare_content)
end

compare_content looks like this:
# Compare the content of a file. Returns true if they are the
same, false if they are not.
def compare_content
checksum(@current_resource.path) == new_resource_content_checksum
end

new_resource_content_checksum looks like this:
def new_resource_content_checksum
@new_resource.content && Digest::SHA2.hexdigest(@new_resource.content)
end

and new_resource.content appears not to be set for cookbook_file.

What am I missing?

On Sat, Dec 8, 2012 at 10:03 PM, Bryan McLellan btm@loftninjas.org wrote:

On Sat, Dec 8, 2012 at 6:46 PM, Noah Kantrowitz noah@coderanger.net wrote:

file “foo” do
owner "whatever"
content { ::File.new(“other”) }
end

That’s too complex. “cp foo bar” is easy, we shouldn’t have to get into ruby
do accomplish that, nor fall back on the execute resource for something this
common and simple.

Thoughts on idempotency?

Copy could check checksums and copy if they differ or perms differ, sort of
like cookbook_file.
Move could be only if the source file (path) exists.

Bryan


#9

I have move the functionality of cookbook_file into file, and expanded
it to resolve CHEF-3676 (copy and move for file resource) and
CHEF-1716 (local for cookbook_file resource).
I still need to update the unit tests and run them, but would
appreciate if one of you fine folks could take a look at what I’ve
done so far, and let me know where I may have gone wrong. (I have
tested local and cookbook files, local and cookbook templates, and
content specified with the content attribute)

Take a look here:

On Sun, Dec 9, 2012 at 12:27 PM, Jesse Campbell hikeit@gmail.com wrote:

Okay… nevermind, i found it, there is a check for whether
cookbook.preferred_filename_on_disk_location returns nil. Yay for good
comments :stuck_out_tongue:

On Sun, Dec 9, 2012 at 10:44 AM, Jesse Campbell hikeit@gmail.com wrote:

I’m taking an initial stab at combining cookbook_file and file.

Before I go too much further, do any of you know how idempotency is
working with cookbook_file?

I see this in the create:
if file_cache_location && content_stale?

content_stale looks like this:
def content_stale?
( ! ::File.exist?(@new_resource.path)) || ( ! compare_content)
end

compare_content looks like this:
# Compare the content of a file. Returns true if they are the
same, false if they are not.
def compare_content
checksum(@current_resource.path) == new_resource_content_checksum
end

new_resource_content_checksum looks like this:
def new_resource_content_checksum
@new_resource.content && Digest::SHA2.hexdigest(@new_resource.content)
end

and new_resource.content appears not to be set for cookbook_file.

What am I missing?

On Sat, Dec 8, 2012 at 10:03 PM, Bryan McLellan btm@loftninjas.org wrote:

On Sat, Dec 8, 2012 at 6:46 PM, Noah Kantrowitz noah@coderanger.net wrote:

file “foo” do
owner "whatever"
content { ::File.new(“other”) }
end

That’s too complex. “cp foo bar” is easy, we shouldn’t have to get into ruby
do accomplish that, nor fall back on the execute resource for something this
common and simple.

Thoughts on idempotency?

Copy could check checksums and copy if they differ or perms differ, sort of
like cookbook_file.
Move could be only if the source file (path) exists.

Bryan


#10

On Sun, Dec 9, 2012 at 1:44 PM, Jesse Campbell hikeit@gmail.com wrote:

Take a look here:
https://github.com/jcam/chef/commits/expanded_file_resource

You could create pull request for this sort of patch, even if you’re
not done. That would allow people to comment in the PR against the
specific parts of the code.

It looks like #action_create no longer differentiates between creating
a file that doesn’t exist and updating the content of a file that does
exist. This seems minor, but my gut tells me it is important for Chef
to know the difference so we can be completely honest with the user
about what we did or are going to do, for purposes like whyrun,
auditing, reporting, etc.

I don’t think I like overloading the ‘create’ action to also be a
’copy’ action, differing on if a source file exists or not, as we’re
not really creating a file.

In #action_move, you remote the source file if it already matches the
destination file. Conceptually this makes sense, but I’d be pretty
surprised if I asked Chef to move a file and it told me that it
deleted it instead.

#copy reports “create a new cookbook_file #{@new_resource.path}” but
is used in both Chef::File#action_move and Chef::File#action_create
(when copying a file). Again, we need to be accurate in what we’re
reporting.

Personally the ‘to’ attribute made more sense to me than 'source.'
When I think about what I’m copying or what I’m moving, I think what I
am describing is the source, not the destination. But I can totally
see how this is possibly personal preference. We see this in
remote_file, cookbook_file and template today. The resource manages
the destination and there is a source attribute. In link, where we
already deal with two files on disk, we use ‘to’ since we realized
this made more sense in CHEF-30 [1].

Bryan

[1] http://tickets.opscode.com/browse/CHEF-30


#11

I like to think of the resource as the thing that is getting created,
not as where it is coming from (otherwise i’d be putting “source” in
the name attribute and in to I would put the destination file that i
want created.

I agree it may make sense to differentiate between create and
replace/update and i can start poking at that.

in my mind though, chef is creating a file, regardless of where the
content comes from, be it a string attribute, a local filesystem file,
a cookbook file, or a remote file. And whether to do template
processing on it should just be a true/false attribute

good to hear a different perspective on these things though, is why i
brought it up

On Mon, Dec 10, 2012 at 1:07 PM, Bryan McLellan btm@loftninjas.org wrote:

On Sun, Dec 9, 2012 at 1:44 PM, Jesse Campbell hikeit@gmail.com wrote:

Take a look here:
https://github.com/jcam/chef/commits/expanded_file_resource

You could create pull request for this sort of patch, even if you’re
not done. That would allow people to comment in the PR against the
specific parts of the code.

It looks like #action_create no longer differentiates between creating
a file that doesn’t exist and updating the content of a file that does
exist. This seems minor, but my gut tells me it is important for Chef
to know the difference so we can be completely honest with the user
about what we did or are going to do, for purposes like whyrun,
auditing, reporting, etc.

I don’t think I like overloading the ‘create’ action to also be a
’copy’ action, differing on if a source file exists or not, as we’re
not really creating a file.

In #action_move, you remote the source file if it already matches the
destination file. Conceptually this makes sense, but I’d be pretty
surprised if I asked Chef to move a file and it told me that it
deleted it instead.

#copy reports “create a new cookbook_file #{@new_resource.path}” but
is used in both Chef::File#action_move and Chef::File#action_create
(when copying a file). Again, we need to be accurate in what we’re
reporting.

Personally the ‘to’ attribute made more sense to me than 'source.'
When I think about what I’m copying or what I’m moving, I think what I
am describing is the source, not the destination. But I can totally
see how this is possibly personal preference. We see this in
remote_file, cookbook_file and template today. The resource manages
the destination and there is a source attribute. In link, where we
already deal with two files on disk, we use ‘to’ since we realized
this made more sense in CHEF-30 [1].

Bryan

[1] http://tickets.opscode.com/browse/CHEF-30


#12

pull request submitted, comment added to ticket:


http://tickets.opscode.com/browse/CHEF-3676

On Mon, Dec 10, 2012 at 1:07 PM, Bryan McLellan btm@loftninjas.org wrote:

On Sun, Dec 9, 2012 at 1:44 PM, Jesse Campbell hikeit@gmail.com wrote:

Take a look here:
https://github.com/jcam/chef/commits/expanded_file_resource

You could create pull request for this sort of patch, even if you’re
not done. That would allow people to comment in the PR against the
specific parts of the code.

It looks like #action_create no longer differentiates between creating
a file that doesn’t exist and updating the content of a file that does
exist. This seems minor, but my gut tells me it is important for Chef
to know the difference so we can be completely honest with the user
about what we did or are going to do, for purposes like whyrun,
auditing, reporting, etc.

I don’t think I like overloading the ‘create’ action to also be a
’copy’ action, differing on if a source file exists or not, as we’re
not really creating a file.

In #action_move, you remote the source file if it already matches the
destination file. Conceptually this makes sense, but I’d be pretty
surprised if I asked Chef to move a file and it told me that it
deleted it instead.

#copy reports “create a new cookbook_file #{@new_resource.path}” but
is used in both Chef::File#action_move and Chef::File#action_create
(when copying a file). Again, we need to be accurate in what we’re
reporting.

Personally the ‘to’ attribute made more sense to me than 'source.'
When I think about what I’m copying or what I’m moving, I think what I
am describing is the source, not the destination. But I can totally
see how this is possibly personal preference. We see this in
remote_file, cookbook_file and template today. The resource manages
the destination and there is a source attribute. In link, where we
already deal with two files on disk, we use ‘to’ since we realized
this made more sense in CHEF-30 [1].

Bryan

[1] http://tickets.opscode.com/browse/CHEF-30