Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Allow knife to install cookbooks with metadata.json #2345

Closed
wants to merge 2 commits into from
Closed

Allow knife to install cookbooks with metadata.json #2345

wants to merge 2 commits into from

Conversation

sethvargo
Copy link
Contributor

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

@sethvargo
Allow knife to install cookbooks with metadata.json
74445de 
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).
@sethvargo sethvargo mentioned this pull request Nov 3, 2014
Closed
sethvargo
sethvargo reviewed Nov 3, 2014
View reviewed changes
lib/chef/knife/cookbook_site_install.rb
return md
end

raise "No metadata.rb or metadata.json!"
Copy link
Contributor Author

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?

Copy link
Contributor

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.

@sethvargo
Copy link
Contributor Author

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 skip_deps)

@adamedx
Copy link
Contributor

adamedx commented Nov 3, 2014

@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.

@adamedx
Copy link
Contributor

adamedx commented Nov 3, 2014

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...

@sethvargo
Copy link
Contributor Author

@adamedx @nathenharvey can explain the story a bit more, but there's a decent amount of context on chef-boneyard/stove#59.

@lamont-granquist
Copy link
Contributor

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.

@sethvargo
Copy link
Contributor Author

@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.

@nathenharvey
Copy link
Contributor

This patch to knife will resolve Issue #9 of the openssl cookbook.

@nathenharvey nathenharvey mentioned this pull request Nov 4, 2014
Closed
@sethvargo
Copy link
Contributor Author

@adamedx @lamont-granquist @nathenharvey let me know if there's anything else you would like to see here 😄

@tyler-ball
Copy link
Contributor

👍

1 similar comment
@lamont-granquist
Copy link
Contributor

👍

@sethvargo
Copy link
Contributor Author

@lamont-granquist who gets the golden ticket to merge this 😄?

Also, I just realized this is PR #2345, which really made my OCD happy

@sersut
Copy link
Contributor

sersut commented Nov 5, 2014

We'll get this merged in our next merge pass @sethvargo

@lamont-granquist
Copy link
Contributor

lolololol on #2345

@lamont-granquist
Copy link
Contributor

OCD

@pburkholder pburkholder mentioned this pull request Nov 14, 2014
Closed
@tyler-ball tyler-ball added this to the 12.2.0 milestone Nov 18, 2014
@sethvargo
Copy link
Contributor Author

Any chance we could backport this to 11-stable as well?

@cwebberOps cwebberOps mentioned this pull request Dec 4, 2014
Closed
@sethvargo
Copy link
Contributor Author

@lamont-granquist is this going to make it in any time soon?

@lamont-granquist
Copy link
Contributor

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.

@btm btm mentioned this pull request Dec 15, 2014
Merged
@btm
Copy link
Contributor

btm commented Dec 16, 2014

This was merged into master/12-stable for release soon in 12.0.2 via #2642. Thanks @sethvargo!

@btm btm modified the milestones: 11.18.0, 12.1.0 Dec 16, 2014
@btm
Copy link
Contributor

btm commented Dec 16, 2014

I've changed the milestone for this pull to 11.18.0 to mark it to get backported into 11-stable.

@dgorack dgorack mentioned this pull request Jan 3, 2015
Closed
jaym added a commit that referenced this pull request Jan 13, 2015
@jaym
Allow knife to install cookbooks with metadata.json
eff465b 
Backport of PRs #2642 and #2345 to chef 11-stable
@jaym jaym mentioned this pull request Jan 13, 2015
Merged
jaym added a commit that referenced this pull request Jan 13, 2015
@jaym
Update Changelog to reflect resolution of #2345
01ff4f9
@someara someara mentioned this pull request Jan 13, 2015
Closed
@martinb3
Copy link
Contributor

👍 I've got some users of my cookbooks reporting this bug as well.

@martinb3 martinb3 mentioned this pull request Jan 14, 2015
Closed
@sethvargo
Copy link
Contributor Author

@jdmundrawala I believe this can be closed? Or did we only backport the change and not merge to master?

@jaym
Copy link
Contributor

jaym commented Jan 14, 2015

@sethvargo It was released in a version of 12. I will close this once 11.18.0 goes out the door.

@jaym
Copy link
Contributor

jaym commented Jan 14, 2015

12.0.2 is what @btm said above

@sethvargo
Copy link
Contributor Author

kk. I just wanted to make sure you weren't waiting on anything from me 😄

@sethvargo
Copy link
Contributor Author

@nathenharvey I think there's an open ticket on a cookbook (maybe openssl) that refers to this issue that can probably be closed too.

@jaym
Copy link
Contributor

jaym commented Jan 15, 2015

Released in 11.18.0

@jaym jaym closed this Jan 15, 2015
@jaym jaym deleted the sethvargo/site_install_json branch January 15, 2015 17:32
@jtimberman jtimberman mentioned this pull request Mar 10, 2015
Closed
@vwxyz vwxyz mentioned this pull request Oct 24, 2015
Closed
@chef chef locked and limited conversation to collaborators Nov 16, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
Type: Enhancement Adds new functionality.
Projects
None yet
Milestone
11.18.0
Development

Successfully merging this pull request may close these issues.

None yet