-
Notifications
You must be signed in to change notification settings - Fork 98
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
Made change to read metadata.json before metadata.rb #39
Conversation
…gument to from_json to be a filename instead of the actual content
It's actually done this way because Chef itself prefers metadata.rb over metadata.json: https://github.com/opscode/chef/blob/master/lib/chef/cookbook/cookbook_version_loader.rb#L59 |
The proposed suggestion in the referenced test-kitchen pull request is to generate a metadata.json, which doesn't solve the problem unless chef-zero reads metadata.json first. Would it be possible for you to review the discussion in that pull request (test-kitchen PR #212) to see if you have any thoughts? |
Additionally, other than the read order of the metadata, there is a bug in the use of the from_json method where it's being passed the contents of a file instead of the filename. |
@jkeiser any reason that Chef is preferring the metadata.rb over metadata.json? It sounds like a mistake to me. Why even generate the metadata.json from the metadata.rb if the Chef server only cares about the metadata.rb, know what I mean? |
@reset my understanding is that metadata.json is a vestige of an earlier era. @danielsdeleo has some context and may be able to help sort things out, here. My primary concern is that chef-zero and chef should agree on the metadata. |
@jkeiser absolutely, I completely agree with you on that ;) I believe that metadata should be compiled and distributed and not distributed and evaluated. It's understandable that other Chef relics like roles (gross) wouldn't need to be compiled into json, but any idea why the |
The JSON data is all that the Chef Server [API] cares about. It doesn't do anything else with the metadata.rb. The Ruby DSL is used for humans to have a nicer way to write the data out without having to worry about the syntax of JSON (and effing commas ;)), nor all the various fields that are (or were in the past) required, like json_class. The Ruby DSL for roles and environments is exactly the same. The Ruby is there for humans to use and write. The server only cares about the rendered JSON. In any case, the client-side does the processing of the Ruby -> JSON. Whether that is |
Okay, so the promised history lesson: Back in the before times (Chef 0.7.x and before), the chef server served cookbooks from a chef repo on-disk, and didn't have an upload mechanism at all. So you would run a rake task that would evaluate metadata.rb, make a JSON equivalent, and rsync it over to the directory where the server would look for cookbooks. Chef 0.8 did more or less the same thing, except you could upload cookbooks to the server as tarballs. When you upload to the server in 0.9 and later, the uploader code is responsible for evaluating metadata.rb and sending the JSON equivalent to the server, so it's not necessary to render a metadata.json file on disk. We eventually removed the generation of metadata.json locally because it confused people and required them to do more configuration of their version control to ignore it (which is easy enough in git and terrible in svn). |
@danielsdeleo @jkeiser yeah I completely agree that leaving the compiled json files on disk is the wrong way to go and support the approach of compiling and uploading them. What @jkeiser is suggesting is that Chef isn't actually using the compiled json but rather prioritizes on the Ruby representation of the metadata. At least, that's what this line of code is saying: https://github.com/opscode/chef/blob/master/lib/chef/cookbook/cookbook_version_loader.rb#L59. |
Another, more testable way to put this: "If your cookbook has metadata.rb with version 1.0.0 and metadata.json with version 2.0.0, knife cookbook upload and knife upload will upload the cookbook as version 1.0.0." I have not tested this; this statement is based on code inspection of the aforementioned code. The question we will have to answer is, is this correct behavior, or should we upload 2.0.0/toss an error? IMO, if a user has a metadata.rb file and .json is generally generated from the .rb, the .rb should be considered the master--the true source of data. So I don't think 2.0.0 would be right. Could be that we should toss an error, though, saying metadata.rb and metadata.json can't coexist ... |
@jkeiser I don't think we should ever be exposing the |
Makes sense to me. So remove the .json case from BOTH, or at least give a deprecated warning when we find json. |
@jkeiser that's the way that I would go |
@jkeiser I really think this one should be merged in. I've made changes to Berkshelf and Ridley to strip the raw metadata on vendor/upload to ensure that we aren't having the Chef Client evaluate raw metadata. Here's two tickets I filed a month back: There's absolutely no reason that the Chef Client should be ever trying to evaluate raw metadata |
CHEF-4811 seems like the relevant one. I really don't want chef-zero to lead Chef; once it is decided that chef-client and knife will prefer metadata.json over metadata.rb (whether a patch is merged or not), I'll gladly switch it over. If for the moment you would like to make a patch that makes this configurable, I'm cool with that! I don't want to stymie your work. |
@jkeiser I don't see the harm in preferring the compiled metadata over the raw metadata ahead of Chef core. The compiled metadata is always more canonical compared to the raw metadata - It's generated for you on upload by Knife. I think making this a configurable option is just going to quickly become a setting that we tell everyone to set out of the box. |
@reset I'm not sure whether it's actually treated as canonical right now. Do you know whether this statement is true or false? "If your cookbook has metadata.rb with version 1.0.0 and metadata.json with version 2.0.0, knife cookbook upload and knife upload will upload the cookbook as version 1.0.0." |
@jkeiser it's canonical in the sense that it is generated for you at upload time and, unlike the raw metadata, it doesn't have the ability to mutate at runtime. Cookbooks from olden days had the option of storing their metadata as compiled ( |
Hi. Your friendly Curry bot here. Just letting you know that there are commit authors in this Pull Request who appear to not have signed a Chef CLA. The following GitHub users do not appear to have signed a CLA: |
.rb vs .json notwithstanding, the fix on the filename is definitely valuable. I was about to make the change as well when I saw this PR. |
@@ -31,15 +31,15 @@ def self.metadata_from(directory, name, version, recipe_names) | |||
# If both .rb and .json exist, read .rb | |||
# TODO if recipes has 3 recipes in it, and the Ruby/JSON has only one, should | |||
# the resulting recipe list have 1, or 3-4 recipes in it? | |||
if has_child(directory, 'metadata.rb') | |||
if has_child(directory, 'metadata.json') | |||
metadata.from_json(File.join(directory.file_path, 'metadata.json')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed this on my local install by using the filename function that's used below. Does it make sense to use that function here as well? Or is File.join safer, etc.?
Hi. I am an automated pull request bot named Curry. There are commits in this pull request whose authors are not yet authorized to contribute to Chef Software, Inc. projects or are using a non-GitHub verified email address. To become authorized to contribute, you will need to sign the Contributor License Agreement (CLA) as an individual or on behalf of your company. You can read more on Chef's blog. GitHub Users Who Are Not Authorized To ContributeThe following GitHub users do not appear to have signed a CLA: |
Reading what's been written about this around the Internet, it seems like the proper solution is to make sure that when the server has JSON metadata, it is honored--which is not the same thing as The solution here is a spec, either way. Either |
just merged #251 didn't see this one, but this has (finally) been merged. tested on commandline and it fixed chef-zero for me. i didn't see anything on .metadata.json.rb, but this fixed a definite bug, so that may be a separate bug lurking to be fixed. |
In addition, fixed the argument to from_json to be a filename instead of the actual content.
The issue that led to this change is the desire to use metadata.json discussed in this PR:
test-kitchen/test-kitchen#212
In order for chef-zero to use the generated metadata.json we had to make the following change to prefer the .json version over .rb. We also discovered that the from_json method requires a file path instead of the JSON contents which were originally being passed to it.