An Artifact?

[cwebberOps]

On the Supermarket, are the cookbooks artifacts? My gut answer to this question is yes, but once I start to add additional context, it gets way fuzzier.

First, a bit of backstory... Version 3.2.3 of stove only uploads a metadata.json and does not upload a metadata.rb. The logic as to why is sound, assuming the cookbook is an artifact, you shouldn't need to, or want to execute arbitrary code on your system. And for Berkshelf, this worked fine. But for others, this resulted in two issues:

1. Foodcritic, run by Fieri, now shows a violation because the metadata.rb is missing. (as an aside, this is easy to address by just telling foodcritic not to care about that specific rule.)
2. knife cookbook site install is now broken when a cookbook that doesn't have a metadata.rb is being installed directly or as part of the dependency chain.

More of the specifics of this discussion can be found in chef/chef#2345.

So for me, this discussion really revolves around a more central question. How do we test quality of a cookbook if it is "just" an artifact?

I see basically two approaches to this problem:

1. In order to get quality ratings for your cookbook, you have to provide a source URL and we then slurp that down and run tests, etc.
2. Run the quality tests against the artifact and know that there will be folks that disagree with that approach.

Each of these approaches has a bit more discussion involved, but I am trying to get an idea what direction to head. What are your thoughts?

[cwebberOps]

The key idea here being that as we move forward with adding more metrics about cookbooks so we can better reason about quality, we need to run things like foodcritic or rspec against something...

[lamont-granquist]

Well the problem with uploading the metadata.rb to supermarket was that the chef-client would consume that on download. Its really a hack to force chef-client not to use it. Supermarket should probably enforce that uploaded cookbooks have both, and then we need to fix the bug in chef-client.

[danielsdeleo]

AFAIK, chef-client doesn't consume the metadata.rb on download (maybe it did at one time and this behavior was fixed by an unrelated patch). I tried to repro this by adding code to raise an exception in a metadata.rb and was unable to trigger it. I think you do see this behavior with Chef Zero and so probably with local mode though. I'm not sure about solo. Anyway, this is a bit in the weeds for the current discussion.

What you're really talking about here is whether it's okay for the metadata.rb file to contain side-effecting code with external dependencies, an in particular whether it's okay for metadata.rb to have code that shells out to git (and will break if you are on a machine without git). And if we think that's cool, then should we strip the metadata.rb when we move cookbooks from place to place so nothing blows up trying to read it? I think it's kinda ugly, but it's the most pragmatic way to go.

[lamont-granquist]

Well seems like its the fault of the thing that reads it and blows up.

[leftathome]

I think the cookbook should be considered an artifact once it's available on Supermarket. If there are static analysis / cookbook quality / doc generation steps being fired, then the cookbook probably needs to be in some kind of "pending approval"/unreleased state until all of those return 0.

[stephenlauck]

TL;DR I think cookbooks on Supermarket should be artifacts and only have metadata.json.

I think this goes back to what we discussed at Summit about a more unified/accepted way to test cookbooks and deliver them as artifacts. I like the idea of the cookbook containing a conventional set of tests that we run when delivering from Git -> Supermarket. We could add this to the "conventions of a good cookbook" thining and only accept cookbooks that have tests into Supermarket or at least rate them as 'better' if they do. I agree when delivered to Supermarket or even an 'Artifact' Chef EC Org there should only be metadata.json which signifies it's been tested/vendored. Cookbooks should not again be tested after pulling from Supermarket and if we nail the above conventions I mention, there would be a level of trust in using community cookbooks from Supermarket.

[lamont-granquist]

I see the point of requiring metadata.json, I'm just not sure I agree with stripping metadata.rb. The metadata.rb just carries additional information with it and could be useful to see how the metadata.json was built. And it feel like requiring the metadata.json addresses all the requirements that its an artifact and tested and finalized, etc.

[charlesjohnson]

+1 to having Supermarket enforce that uploaded code has both metadata.rb and metadata.json. Further I think this needs to happen quickly, and that cookbooks without metadata.rb that are currently stored on the Supermarket need to be found and remediated as quickly as possible, because users are bearing the brunt of the issue.

To be clear: I'm on-board with preferring metadata.json on the consumer side, and I'm on-board with all the arguments in Jamie's post. I think he's right and this is clearly the right thing to do. That said:

Right now not having metadata.rb in a cookbook on Supermarket breaks knife cookbook site install if that cookbook is in a dependency chain. This is true on all versions of knife as far as I know, which is unacceptable. Worse, people who encounter this issue are currently helpless to fix it themselves. It's not like they can submit a metadata.rb for someone else's breaking cookbook.

The proper fix here is probably for knife cookbook site to get some love and function correctly in the absence of metadata.rb, but even once that's released, allowing cookbooks without metadata.rb is a change that will broadly break backward compatibility. It's a big enough change that even foodcritic's base ruleset will need to be updated.

I feel there are enough ramifications here that a change to the cookbook spec is probably needed to specify that metadata.json be preferred for consumption and that metadata.rb not be distributed beyond the source, and that change should probably flow through the RFC process.

The only argument I can see that carries weight for making the change immediately and unilaterally is safety: If we need to protect people from an attack vector, then that's cause for quick action. However I haven't seen anything I feel warrants that kind of movement.

[yvovandoorn]

👍 on making this flow through a RFC process, and in my opinion, it should address, where ever possible, the entire workflow (e.g. certain tools preferring metadata.json vs metadata.rb) at once.

[coderanger]

+1 on cookbooks are artifacts just like gems or other released packages.

-1 on any attempt to require adherence to Foodcritic or other static analysis tools (my code almost universally makes foodcritic cranky)

-0 on requiring metadata.rb until we are really certain nothing will use it, +0 after that. We should basically follow the same rules as PyPI (for wheels) and RubyGems, uploaded packages include the setup.py or gemspec, but tooling doesn't use them.

[alexmanly]

+1 to having Supermarket enforce that uploaded code has both metadata.rb and metadata.json. It breaks the workflow of downloading cookbooks from public supermarket and then uploading them to a private supermarket for those customer that want to contain all packages/automation within the firewall.

[nellshamrell]

Thank you everyone for this discussion! This will probably continue into the next year and should be addressed with an RFC, we will definitely keep this issue in mind. Thank you so much for the input!