search

chef / chef_backup

A library to backup an Chef server
Other
7 stars 15 forks source link

Don't require chef by vendoring deep_merge #20

Closed stevendanna closed 8 years ago

stevendanna commented 8 years ago

Chef is a big dependency to pull in for 1 file. Also 'di-ruby-lvm' appears unused.

sdelano commented 8 years ago

Are there tests from deep_merge that we should pull over as well? I remember there being some bugs in this library at one time, and adding the associated tests to this library would be beneficial in the eventual future where somebody modifies this without contributing or pulling from the upstream.

stevendanna commented 8 years ago

@sdelano Good call, I'll move them over if I can get them running without also bringing over all of chef's test machinery. They look pretty straightforward so I don't think it'll be a problem.

stevendanna commented 8 years ago

Specs for Mash and DeepMerge also vendored.

marcparadise commented 8 years ago

This is an odd case. This gem can only be used in places where chef is already installed as part of a server package, and will only be used in contexts where it's available to be 'required'. It may be a simpler/safer option to document that, and just remove the dependency?

marcparadise commented 8 years ago

Alt 2 - something to consider down the road. I took a closer look at deep_merge usage, and I'm not clear that we actually need it at all - unless I missed something, it's used only near the end of the tar restore strategy to merge running_config into ChefBackup::Config, and after that point the only used value is restore_dir for import_db. Alt 3: Also for down the road, but given the usage constraints of this gem, it might make sense to just integrate it into chef-server itself?

In any case I'm :+1: if y'all think it's best to maintain this here as well, but given the usage profile I think it can be avoided via one of the options here or my initial suggestion.

stevendanna commented 8 years ago

This is an odd case. This gem can only be used in places where chef is already installed as part of a server package, and will only be used in contexts where it's available to be 'required'. It may be a simpler/safer option to document that, and just remove the dependency?

This gem only needs to call chef-client as an executable. I'd like to keep chef out of the dependency tree so that we can appbundle and application that include this and not have to pay any of the runtime costs of having chef in our gempath when other parts of that program are being run.

If we don't want to vendor, than I think the most sensible course is to just remove it since it is only used to merge two config structures.

stevendanna commented 8 years ago

cc @sdelano @ryancragun Thoughts? I'd like to move forward on this either way.

sdelano commented 8 years ago

I'm 👍 with this approach. There's no clear winner in terms of implementation, and if this change makes things faster during execution then I'm on board with it.