-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Allow knife to install cookbooks with metadata.json #2345
Conversation
Previously `knife cookbook site install` would only install cookbooks that have a metadata.rb. This has a number of problems, mainly that compiled metadata is supported elsewhere in Chef. This commit adds support for parsing the metadata.json, even though the metadata.rb is still preferred (since that's what the rest of Chef does).
return md | ||
end | ||
|
||
raise "No metadata.rb or 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 couldn't find a "real" exception class to raise here. What should it be?
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.
There is MetadataNotValid but I think we should create MetadataMissing
right next to that:
class MetadataMissing < StandardError
def initialize
super "No metadata.rb or metadata.json!"
end
end
That way it has a default error message that can be overridden.
There are currently no tests, mostly because there is already little test coverage. I couldn't find a good way to simulate the loading of these deps (all the current tests just use |
@sethvargo, thanks for the contribution. I'm assuming that berkshelf uses metadata.json for the same scenario, so this is ensuring that this command can also conform, correct? @dan, fyi in case this has implications for other cookbook management tools. |
For tests, I think we'd just like to have unit specs that state the supported behaviors (metadata.rb and metadata.json support) and validate that it hits the right code paths in the right circumstances. So mocking out stuff to hit that should be sufficient. I'm guessing the low test coverage is a result of this being an example of some fairly early Chef / knife code... |
@adamedx @nathenharvey can explain the story a bit more, but there's a decent amount of context on chef-boneyard/stove#59. |
Yeah LGTM modulo the real exception. I expect adding tests would be fairly annoying. We probably really need a small supermarket-zero built on top of TinyServer to do func tests against. |
@lamont-granquist @tyler-ball added the real exception class. @adamedx I tried writing tests, but it's really difficult to mock all this knife stuff out 😦. I don't see of a clean or even sane way to test this. |
This patch to knife will resolve Issue #9 of the openssl cookbook. |
@adamedx @lamont-granquist @nathenharvey let me know if there's anything else you would like to see here 😄 |
👍 |
1 similar comment
👍 |
@lamont-granquist who gets the golden ticket to merge this 😄? Also, I just realized this is PR #2345, which really made my OCD happy |
We'll get this merged in our next merge pass @sethvargo |
lolololol on #2345 |
Any chance we could backport this to 11-stable as well? |
@lamont-granquist is this going to make it in any time soon? |
It missed the deadline to get merged into 12.0.0 (that occurred a long time ago and then there was a whole lot of yak shaving before we could ship 12.0.0 due to fixing regressions and performance bugs). It'll ship in 12.1.0, but we've got the 12.0.1 yak to shave first. We also have to release a final chef-11 version of chef-dk and then a chef-12-only chef-dk. We've got a backlog of ready-to-merge, but that has just been a lower priority lately since it all goes in 12.1.0 and 12.1.0 is backed up on all-the-things. Hopefully we get a goalie with some time to do md-files and merge all this soon, but it won't make 12.1.0 get out any faster either way. |
This was merged into master/12-stable for release soon in 12.0.2 via #2642. Thanks @sethvargo! |
I've changed the milestone for this pull to 11.18.0 to mark it to get backported into 11-stable. |
👍 I've got some users of my cookbooks reporting this bug as well. |
@jdmundrawala I believe this can be closed? Or did we only backport the change and not merge to master? |
@sethvargo It was released in a version of 12. I will close this once 11.18.0 goes out the door. |
12.0.2 is what @btm said above |
kk. I just wanted to make sure you weren't waiting on anything from me 😄 |
@nathenharvey I think there's an open ticket on a cookbook (maybe openssl) that refers to this issue that can probably be closed too. |
Released in 11.18.0 |
Previously
knife cookbook site install
would only install cookbooks that have a metadata.rb. This has a number of problems, mainly that compiled metadata is supported elsewhere in Chef.This commit adds support for parsing the metadata.json, even though the metadata.rb is still preferred (since that's what the rest of Chef does).
Refs: chef-boneyard/stove#59
/cc @nathenharvey