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

Conversation

Projects
None yet
8 participants
@coderanger
Copy link
Contributor

coderanger commented Nov 9, 2014

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

@danielsdeleo

This comment has been minimized.

Copy link
Member

danielsdeleo commented Nov 10, 2014

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

This comment has been minimized.

Copy link
Contributor

coderanger commented Nov 10, 2014

@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

This comment has been minimized.

Copy link
Member

danielsdeleo commented Nov 10, 2014

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

This comment has been minimized.

Copy link
Contributor

coderanger commented Nov 10, 2014

@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

This comment has been minimized.

Copy link
Contributor

lamont-granquist commented Nov 10, 2014

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

This comment has been minimized.

Copy link
Contributor

coderanger commented Nov 10, 2014

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

This comment has been minimized.

Copy link
Contributor

lamont-granquist commented Nov 10, 2014

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

This comment has been minimized.

Copy link
Contributor

coderanger commented Nov 10, 2014

+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

This comment has been minimized.

Copy link
Contributor

coderanger commented Nov 10, 2014

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

@jonlives

This comment has been minimized.

Copy link
Contributor

jonlives commented Nov 11, 2014

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

This comment has been minimized.

Copy link
Contributor

lamont-granquist commented Nov 11, 2014

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

This comment has been minimized.

Copy link
Member

danielsdeleo commented Nov 11, 2014

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

This comment has been minimized.

Copy link
Contributor

lamont-granquist commented Nov 11, 2014

@coderanger

This comment has been minimized.

Copy link
Contributor

coderanger commented Nov 11, 2014

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

This comment has been minimized.

Copy link
Member

danielsdeleo commented Nov 11, 2014

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

This comment has been minimized.

Copy link
Contributor

coderanger commented Nov 11, 2014

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

This comment has been minimized.

Copy link
Contributor

coderanger commented Nov 11, 2014

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

This comment has been minimized.

Copy link
Contributor

lamont-granquist commented Nov 11, 2014

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

This comment has been minimized.

Copy link

sethvargo commented Nov 11, 2014

@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

This comment has been minimized.

Copy link
Member

danielsdeleo commented Nov 11, 2014

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

@kwilczynski

This comment has been minimized.

Copy link

kwilczynski commented Nov 11, 2014

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

This comment has been minimized.

Copy link

sethvargo commented Nov 11, 2014

@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

This comment has been minimized.

Copy link
Contributor

coderanger commented Nov 11, 2014

@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

This comment has been minimized.

Copy link
Member

danielsdeleo commented Nov 11, 2014

@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

This comment has been minimized.

Copy link

sethvargo commented Nov 11, 2014

@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

This comment has been minimized.

Copy link
Contributor

coderanger commented Nov 12, 2014

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,

This comment has been minimized.

@stevendanna

stevendanna Nov 12, 2014

Member

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

@jonlives

This comment has been minimized.

Copy link
Contributor

jonlives commented Nov 12, 2014

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

@coderanger

This comment has been minimized.

Copy link
Contributor

coderanger commented Nov 21, 2014

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

@danielsdeleo

This comment has been minimized.

Copy link
Member

danielsdeleo commented Nov 21, 2014

I'm 👍 on this version.

@lamont-granquist

This comment has been minimized.

Copy link
Contributor

lamont-granquist commented Nov 21, 2014

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

This comment has been minimized.

Copy link
Contributor

coderanger commented Nov 21, 2014

@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

This comment has been minimized.

Copy link
Contributor

lamont-granquist commented Nov 21, 2014

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

This comment has been minimized.

Copy link
Contributor

coderanger commented Nov 21, 2014

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

@lamont-granquist

This comment has been minimized.

Copy link
Contributor

lamont-granquist commented Nov 21, 2014

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

This comment has been minimized.

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

Merge pull request #66 from coderanger/alias
Root Aliases in Cookbooks

@coderanger coderanger merged commit a55d7df into chef:master Nov 26, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment