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.

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

Root Aliases in Cookbooks #66

Merged
merged 2 commits into from Nov 26, 2014
Merged

Conversation

coderanger
Copy link
Contributor

Another up-port from the old dialects branch. /cc @sersut @danielsdeleo @lamont-granquist

@danielsdeleo
Copy link
Contributor

I like the general feature, but I don't like the part of the implementation where we "translate" the cookbook into the "full size layout" on upload. My objections are:

  • I think it will make debugging more complicated, because the filenames we give you will not line up with what you did on your workstation.
  • The cookbook load/upload code is already way more complicated than I would like, because of overlays and other stuff we want to get rid of, and I would prefer not to add more complexity to it.
  • We'd have to special-case for chef-solo somehow.
  • Chef Zero would have to translate on the fly.

This would all be a lot easier if we could just upload the files in the locations they're in. That said, it may not be possible to do that with the current cookbooks APIs which are pretty specific about the different kinds of files that can appear in cookbooks. My opinion about this is that the server API needs to change to tolerate files at any path the user wants. The way things are right now makes certain testing workflows a real PITA and doesn't have a clear benefit (IMO). @lamont-granquist and I have thought through how we might migrate to a "segment agnostic" API a little bit and probably need to write an RFC.

Thoughts?

@coderanger
Copy link
Contributor Author

@danielsdeleo The existing implementation doesn't actually move stuff around, it hook in at the point where the cookbook-relative-to-absolute (and vice versa) path resolution happens.

@danielsdeleo
Copy link
Contributor

Ok, then I probably just didn't understand this line:

When building the cookbook manifest for upload, the alias files in the cookbook root are instead moved to their relevant segment.

@coderanger
Copy link
Contributor Author

@danielsdeleo That is just about the manifest itself, normally anything outside a folder ends up in the root_filenames segment, instead they get added to the semantically correct segment.

@lamont-granquist
Copy link
Contributor

so on the converging chef-client do you get /var/chef/cache/cookbooks/whatever/attributes.rb or do you get /var/chef/cache/cookbooks/whatever/attributes/default.rb or something like that?

@coderanger
Copy link
Contributor Author

You get "/var/chef/cache/cookbooks/whatever/attributes.rb". The tricks are all just in places that resolve symbolic or relative names to actual paths.

@lamont-granquist
Copy link
Contributor

Okay, so the segments need to die. So you don't need to go out of your way to add them to the correct segments. OTOH, if adding them to the correct segments keeps the patch simpler, then that's fine, it just becomes more evidence that we need to kill segments eventually to make these hacks go away.

@coderanger
Copy link
Contributor Author

+1, I don't really know what the file segments stuff was supposed to be for but definitely would be a fan of it going away. Trying to keep this more tightly scoped though.

@coderanger
Copy link
Contributor Author

When I port over the patches I'll see if dropping the segment fiddling makes it easier or not.

@jonlives
Copy link
Contributor

I'm in favour of deprecating attributes files and replacing them with a default (given as @coderanger said they all get loaded anyway) but am not so keen on aliasing libraries and recipes - in the case of libraries I worry it might result in less experienced users cramming multiple class definitions into libraries.rb if they don't realise multiple files are supported.

I guess my main concerns here are adding behaviour that might be fairly opaque to user who don't know what they're looking at.

@lamont-granquist
Copy link
Contributor

Yeah, so I'm okay with this any way it shakes out. Mostly my concern is around maintaing a path forwards to where the segments can get dropped out of cookbook metadata and it starts looking more like a dumber rsync --delete, so as long as we're doing faithful copies of what is in the git repo to the chef-server to what the client is seeing and the layout is consistent, I'm okay.

@danielsdeleo
Copy link
Contributor

I agree with Lamont about the segments stuff, so I think it could make sense to use more generic language about that part of the implementation (otherwise, when we rip out segments, this is either partially obsolete or we'll have to come back and update it).

Also, can you expound upon the library file use case? You wrote:

Including libraries is mostly driven by the growth of ChefSpec and the need for a small library file to define matchers.

Is a library the right place for that? Can you provide an example of a cookbook that does this?

@lamont-granquist
Copy link
Contributor

@coderanger
Copy link
Contributor Author

Yep, that's the use case I had in mind. Definitely could be removed though since it isn't in the same league as attributes and recipes in terms of making simple use cases simple.

@danielsdeleo
Copy link
Contributor

I'm confused as to why the convention is to put that kind of code in libraries/ instead of in spec/, do you know why that is?

@coderanger
Copy link
Contributor Author

No idea, I don't really use it myself I just see it as a common case of one-child-in-a-folder and thus I hate it :)

@coderanger
Copy link
Contributor Author

Also worth noting that we could remove libraries for now, once the code is in place it is much easier to add/remove from the actual alias mappings.

@lamont-granquist
Copy link
Contributor

I can drop those matchers in the spec/ dir and it works fine, and there's no runtime dep on those its just for chefspec, so it doesn't need to be part of the uploaded/runtime cookbook. I'm not certain exactly how that convention got started. Paging @sethvargo i guess.

@sethvargo
Copy link

@danielsdeleo you put them in libraries so Chef will autoload them. These aren't test helpers for you the developer, they are test helpers for other people the developers using your cookbook. For example, I may create a cookbook called "bacon" that defines an LWRP named eat_bacon. I want to also package a custom matcher for my users.

@danielsdeleo
Copy link
Contributor

@sethvargo so the problem is that Chef doesn't package your tests when you upload to Supermarket, etc.?

@kwilczynski
Copy link

I will wholly support what @jonlives said.

I would like to add, that I am not quite sure what real problem this proposal is trying to solve. Perhaps I don't see a wood for a tree, so to speak. For instance, RFC017 (mentioned in the rationale) is incredibly useful, this one not so much, IMHO.

@sethvargo
Copy link

@danielsdeleo it's a two-part problem:

  1. Yes, Chef/Stove/Supermarket/ ignores the spec/ folder when packaging a cookbook
  2. There's no easy mechanism for requiring files from inside the cookbook

Unlike gems, which get added to $:, cookbooks don't. So you can't just require "bacon/rspec" or something to pickup the helpers. Any ideas?

@coderanger
Copy link
Contributor Author

@kwilczynski End goal is easing the learning curve of Chef by having less required boilerplate-y things. Most cookbooks have a single recipe and single attributes file, so there is no reason to make those folders for that case.

@danielsdeleo
Copy link
Contributor

@sethvargo we would like to solve (1) by getting rid of the "segments" concept from the Chef APIs and make cookbooks include whatever files you have. I think this would be most welcome for people doing things like Chef minitest or similar. For (2), I'm open to any solution that works. Off the top of my head, it seems like it should be straightforward for Chef to give you an API to, say, find all of the cookbooks with a spec/ dir and spit out a list of those paths, which ChefSpec could then add to the load path, though if a different solution works better, I'm all for it.

@sethvargo
Copy link

@danielsdeleo I don't have a problem adding a method to ChefSpec either that searches, so long as there is a corresponding public API in Chef I can query 😄

@coderanger
Copy link
Contributor Author

If we are removing libraries from consideration (which it sounds like would be for the best since issues with those files can be better solved elsewhere or are advanced use cases anyway), anyone have strong opinions on just doing attributes.rb and recipe.rb, i.e. skipping the recipes.rb alias? I do like the simplicity of folder name -> file name, but given the use case I'm not sure it is worth bothering.

## Motivation

As a cookbook author,
I want to less complex directory layouts,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: should this be "I want a less complex..."?

@jonlives
Copy link
Contributor

I'd be fine with doing attributes.rb and recipe.rb, those would definitely simplify things IMO.

@coderanger
Copy link
Contributor Author

Nuked libraries for now, and removed the specific mentions of segments so this will still apply in a glorious segment-free future.

@danielsdeleo
Copy link
Contributor

I'm 👍 on this version.

@lamont-granquist
Copy link
Contributor

IMO, we should migrate to a single attributes.rb file format (slowly) and at some point warn on attributes/* and at some point remove them entirely. The fact that there are multiple attributes files, they're all sourced together, they're named 'attributes/default.rb' which matches 'recipes/default.rb' but there's no correlation and there's nothing particularly default about 'attributes/default.rb' compared to 'attributes/foo.rb' makes me just want to get rid of the directory entirely.

OTOH, I feel about recipe.rb roughly the same way as libraries/resources/providers that learning it first violates the law of primary and adds more magic stuff to know and we can never simplify and remove one way or the other.

@coderanger
Copy link
Contributor Author

@lamont-granquist I see the recipe.rb about the same as lib/chef.rb vs. lib/chef/version.rb in Ruby. If we were following that model recipes/default.rb would actually be the bit to be deprecated, though I think it can coexist with the alias for a very long time.

@lamont-granquist
Copy link
Contributor

Yeah, but if you're not allowing declaring multiple recipes in a recipe file (like multiple actions in a provider) then you always have to support a directory structure of recipes, so the single-file-vs-directory-of-files models will always have to co-exist, and i'd rather pick one or the other. The way that multiple recipe are implemented as multiple files also feels sensible to me, along with the use of include_recipe and having that map to files and it all makes sense. I'm not sure a single-file model feels natural and you don't get good separation of concerns there (you can have ubuntu segregated from redhat in different files which feels right to me -- if i'm concerned with ubuntu I often don't want to look at redhat -- not to mention unix-vs-windows).

@coderanger
Copy link
Contributor Author

I agree both need to exist, but I think single-recipe cookbooks are common enough to be worth the special case.

@lamont-granquist
Copy link
Contributor

Having sat with someone just on Wednesday who was largely Chef Naive and having to explain all-the-things from a Chef 101 level, I think all the different special cases have a large cognitive cost.

@adamhjk
Copy link
Contributor

adamhjk commented Nov 25, 2014

Ok - this looks like it has the 👍 from the whole universe. Make it so, @opscode/rfc-editors

coderanger added a commit that referenced this pull request Nov 26, 2014
Root Aliases in Cookbooks
@coderanger coderanger merged commit a55d7df into chef-boneyard:master Nov 26, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
8 participants