search

electric-it / minimart

MiniMart RubyGem for Chef cookbook mirroring and storage.
Apache License 2.0
52 stars 20 forks source link

Handle Ridley cookbook parsing errors #33

Closed AnalogJ closed 8 years ago

AnalogJ commented 8 years ago

Ridley will fail if a cookbook is missing a name attribute in its metadata.rb file, even though chef doesn't require it. Its common practice now, but some older cookbook versions don't have it, which causes issues for us when we mirror community cookbook repos:

https://github.com/chef-cookbooks/apt/blob/1.5.0/metadata.rb

To handle this case I've added a try - catch block around each cookbook fetch, so if a failure occurs when parsing a specific cookbook version, its ignored.

tapickell commented 8 years ago

Can you add a test case for this issue in the specs?

AnalogJ commented 8 years ago

Sure, I'll add a testcase in a moment.

Here's the full Stacktrace

    /removed/gem/ruby/2.1.0/gems/ridley-4.4.2/lib/ridley/chef/cookbook.rb:44:in `from_path': The metadata at '/var/folders/qq/lqzh9_t563x45y58qmm8r9d80000gn/T/d20160217-88384-2ady2n' does not contain a 'name' attribute. While Chef does not strictly enforce this requirement, Ridley cannot continue without a valid metadata 'name' entry. (Ridley::Errors::MissingNameAttribute)
            from /removed/gem/ruby/2.1.0/gems/minimart-1.1.3/lib/minimart/cookbook.rb:19:in `from_path'
            from /removed/gem/ruby/2.1.0/gems/minimart-1.1.3/lib/minimart/inventory_requirement/git_requirement.rb:71:in `block in download_cookbook'
            from /removed/gem/ruby/2.1.0/gems/minimart-1.1.3/lib/minimart/download/git_repository.rb:25:in `call'
            from /removed/gem/ruby/2.1.0/gems/minimart-1.1.3/lib/minimart/download/git_repository.rb:25:in `block in fetch'
            from /opt/chefdk/embedded/lib/ruby/2.1.0/tmpdir.rb:88:in `mktmpdir'
            from /removed/gem/ruby/2.1.0/gems/minimart-1.1.3/lib/minimart/download/git_repository.rb:21:in `fetch'
            from /removed/gem/ruby/2.1.0/gems/minimart-1.1.3/lib/minimart/inventory_requirement/git_requirement.rb:70:in `download_cookbook'
            from /removed/gem/ruby/2.1.0/gems/minimart-1.1.3/lib/minimart/inventory_requirement/base_requirement.rb:47:in `fetch_cookbook'
            from /removed/gem/ruby/2.1.0/gems/minimart-1.1.3/lib/minimart/mirror/inventory_builder.rb:36:in `block in install_cookbooks_with_explicit_location'
            from /removed/gem/ruby/2.1.0/gems/minimart-1.1.3/lib/minimart/mirror/inventory_requirements.rb:31:in `call'
            from /removed/gem/ruby/2.1.0/gems/minimart-1.1.3/lib/minimart/mirror/inventory_requirements.rb:31:in `block in each_with_explicit_location'
            from /removed/gem/ruby/2.1.0/gems/minimart-1.1.3/lib/minimart/mirror/inventory_requirements.rb:26:in `each'
            from /removed/gem/ruby/2.1.0/gems/minimart-1.1.3/lib/minimart/mirror/inventory_requirements.rb:26:in `each'
            from /removed/gem/ruby/2.1.0/gems/minimart-1.1.3/lib/minimart/mirror/inventory_requirements.rb:30:in `each_with_explicit_location'
            from /removed/gem/ruby/2.1.0/gems/minimart-1.1.3/lib/minimart/mirror/inventory_builder.rb:35:in `install_cookbooks_with_explicit_location'
            from /removed/gem/ruby/2.1.0/gems/minimart-1.1.3/lib/minimart/mirror/inventory_builder.rb:19:in `build!'
            from /removed/gem/ruby/2.1.0/gems/minimart-1.1.3/lib/minimart/commands/mirror.rb:22:in `execute!'
            from cookbook_repo.rb:50:in `<main>'
AnalogJ commented 8 years ago

@tapickell Actually I might have jumped the gun when I said I would have a test case ready in a moment. Your test suite is pretty comprehensive, but I'm not sure how to add my test for a faulty metadata.rb file.

tapickell commented 8 years ago

It's cool, let me see if I can help.

tapickell commented 8 years ago

The first thing I would do is create a new fixture to test with. You could duplicate the sample_cookbook directory in spec/fixtures/ and name it bad_metadata_cookbook or something like that. Then remove the name portion of the metadata from the file.

tapickell commented 8 years ago

Next we need to figure out where this would best fit into the specs. Looking through it now.

tapickell commented 8 years ago

In inventory_builder_spec.rb #build, down at the bottom I would add a context of something like "when a cookbook is missing name param in the metadata". Then add a let for bad_cookbook that grabs that cookbook fixture. Then make an assertion in there for the skip.

tapickell commented 8 years ago

That would be good enough for this PR I think. I can work on adding a better error handling solution for this later when I have more bandwidth.

AnalogJ commented 8 years ago

@tapickell Thanks for all the help, I'll try to get that done this afternoon.

If you're looking for some community cookbooks, I got a whole list of popular cookbooks/repos that have mis-labled tags, duplicate versions and invalid cookbooks that you can test against.

We're currently using a chef-server with a berkshelf-api frontend as our internal cookbook repository. I've wanted to move to supermarket for a while but it doesn't really support automation.

I'm really happy with minimart so far, but its a bit more sensitive to mangled cookbooks than chef-server, which is a bit of a problem for us because we mirror a hundred or so community cookbooks, and there's a few that are terrible (I'm looking at you apt, jenkins).

tapickell commented 8 years ago

No problem.

This was initially developed to fit our needs internally and we created a Jenkins job to help automate it for us. If you get any ideas for features that will help you automate it better please feel free to submit more PR's. Always feel free to reach out to me if you need some direction.

We no longer have a team developing this, just Bernie and I supporting it. We are very grateful for the help and reception this project has received from the Chef Community.

Thank you for your contributions.

berniedurfee-ge commented 8 years ago

@AnalogJ Hi and thanks for the contribution!

Looks like a bunch of the existing tests are failing, apologies if the thread above explains why, I haven't fully processed it. I'll try to dig in this week to see if I can fix it. Just want to get Travis green before the merge.

berniedurfee-ge commented 8 years ago

@AnalogJ BTW, also nice to see someone making use of this! :smile:

AnalogJ commented 8 years ago

Yeah, there's a few failures in there that shouldn't be related to the new tests I added. However https://travis-ci.org/electric-it/minimart/jobs/110041546#L273 is definitely my fault.

@tapickell Any idea what I did wrong? I basically copy and pasted most of that test

tapickell commented 8 years ago

I will see if I can dig in and look at the tests later today as well.

AnalogJ commented 8 years ago

I think I found the cause of these test failures

https://github.com/electric-it/minimart/blob/master/spec/lib/minimart/mirror/local_store_spec.rb#L9

Basically the spec/fixtures folder is treated as an inventory directory by the tests, which causes an issue when the web action is run. I've moved my broken cookbook into a sibling directory of the fixtures folder.

My change was to ignore broken cookbooks (without adding them to the inventory), which means they won't ever be in the inventory directory anyways.

AnalogJ commented 8 years ago

@tapickell @berniedurfee-ge @richardardrichard Hey guys, any eta on when I can expect this PR to be merged? The change is pretty basic. Most of the commit is adding tests.

tapickell commented 8 years ago

:+1: