Add skeleton option for chef generate

[martinisoft]

I'd like to add an option to the chef generate command to allow customization of the 'skeleton' cookbook path. Currently it is hard-wired in to the gem and it seems the option is documented in the methods for the generators here and here.

Eventually I'd like to re-factor this a bit so it can simply command the specified generator cookbook in a path to run a given recipe argument giving way to fully custom generators based upon a cookbook. Also, I think it's really cool that this is using a cookbook to build cookbooks 🐢.

[danielsdeleo]

@martinisoft Since the command has no options right now, I think it might be missing the call to parse_options that makes Mixlib::CLI actually parse the options. Probably adding some code like this should fix it:

  # arguments will be any non-option arguments from ARGV
  arguments = parse_options(argv)

[martinisoft]

@danielsdeleo I've tried adding that into the Base parser, but get an invalid option error.

ChefDK::Command::GeneratorCommands::Cookbook
  when given a cookbook skeleton path
    configures the generator context (FAILED - 1)

Failures:

  1) ChefDK::Command::GeneratorCommands::Cookbook when given a cookbook skeleton path configures the generator context
     Failure/Error: cookbook_generator.read_and_validate_params
     OptionParser::InvalidOption:
       invalid option: --skel
     # ./lib/chef-dk/command/generator_commands.rb:216:in `read_and_validate_params'
     # ./spec/unit/command/generator_commands_spec.rb:204:in `block (3 levels) in <top (required)>'

[danielsdeleo]

Ugh, that's because mixlib-cli options don't inherit from the superclass 😦

Something like this might work:

module SharedGeneratorMethods
  include Mixlib::CLI
  option :skeleton # etc.
end

class GeneratorClass
   options.merge!(SharedGeneratorMethods)
end

It's kinda ugly so it'd be preferable to add support directly to mixlib-cli for that kind of thing (even if the implementation ends up being the same).

[martinisoft]

@danielsdeleo Ah great suggestion. Newest commit gets one of the tests to pass, but now it looks like that is creating nil in the run context so I wonder if the option parsing is not working in some superclasses.

Failures:

  1) ChefDK::Command::GeneratorCommands::Cookbook when given a cookbook skeleton path creates a new cookbook
     Failure/Error: cookbook_generator.run
     TypeError:
       no implicit conversion of nil into String
     # ./lib/chef-dk/chef_runner.rb:40:in `run_context'
     # ./lib/chef-dk/chef_runner.rb:36:in `converge'
     # ./lib/chef-dk/command/generator_commands.rb:193:in `run'
     # ./spec/unit/command/generator_commands_spec.rb:214:in `block (4 levels) in <top (required)>'
     # ./spec/unit/command/generator_commands_spec.rb:212:in `chdir'
     # ./spec/unit/command/generator_commands_spec.rb:212:in `block (3 levels) in <top (required)>'

[sethvargo]

• 👍 on the idea of this
• 👎 on naming it "skeleton"

[martinisoft]

@sethvargo Open to suggestions on the name. Went with skel based upon those comments to stick to original intent. I'm thinking along the lines of 'template' similar to Rails.

[danielsdeleo]

I originally picked "skeleton" based on the similarity to stuff like /etc/skel in OS-land, but I'm open to changing it. I don't think "template" is a good name, because that is already an important word with a particular meaning in Chef. Maybe "cookbook" or "generator" ?

[sethvargo]

I don't think "template" is a good name, because that is already an important word with a particular meaning in Chef.

I actually disagree. It's all about context. Rails calls their custom generator things "templates", and Rails also has a notion of "templates" in view.

If I can't convince you to use template, I think generator is a bad name because it refers to the internals. I would expect generator to change the underlying technology, not the layout.

I think skeleton is bad because it has a negative connotation (death). I also have never seen it used in a developer tool, so I worry about the semantics and non-native speakers. It's not very intuitive. Furthermore, skeleton describes the thing you are generating, not the template to use for generation.

cookbook is too Cheffy. ChefDK is targeting people who may not be familiar with Chef, so using Chef-specific terminology will be less than intuitive.

I really think template is the right choice here. I understand that it has a namespace conflict with Chef land, but I think it's in line with many other project generators (rails, django, codeigniter). I think it will reduce the barrier to entry and it just "feels right" to me.

[danielsdeleo]

@sethvargo My specific concern about "template" is that it makes it sound like you can write just the template you want to use for a particular file, e.g., chef generate recipe foo --template my_recipe.erb whereas you really need a cookbook with a specific structure.

Also, I expect that custom generators will be more of an intermediate to advanced feature, so I don't think it's a concern to be "too Cheffy." By the time you use this you should either know the basics of Chef (or maybe be a new employee following instructions from someone else who knows Chef). I also don't think that web frameworks will necessary be a shared cultural context we can expect of everyone. What about operations people who are moving from bash/perl scripting to programming via Chef?

[sethvargo]

What about something longer, but more descriptive? Like --generator-cookbook?

[danielsdeleo]

@sethvargo that works for me, if paired with a short option like -g or something.

[sethvargo]

👍 Aaron?

[martinisoft]

Agreed on both counts. Incoming commits to change and fix that. Thank you @danielsdeleo and @sethvargo

[martinisoft]

Cleaned up a bit more of the code and tidy'd up the inline documentation.

[sethvargo]

[Pendantic] I would say "PATH" over "DIR". I think it's clearer

[sethvargo]

This looks good, but I wonder if merging with options is the "right" thing to do.

What if we did something like:

module GeneratorOptions
  def self.included(base)
    base.send(:option, ...)
  end
end

And then include GeneratorOptions. I'm thinking more "in the future". The advantage of the module is that you can check if a class has the options using is_a?:

Command.new.is_a?(GeneratorOptions)

I don't know. What do you y'all think?

[danielsdeleo]

@sethvargo I tried that with knife-ec2 and it kinda sucks, you have to do the whole thing inside class_eval or whatever, plus callbacks are a PITA to debug, and only give the appearance of using OO hierarchies when in reality you're sweeping a bunch of procedural code to mutate a data structure under the rug. We could alternatively semi-monkey-patch by redefining GeneratorClass.options to check the superclass for options. I kinda dislike that because I'd prefer to have an approach that enables composition better, even if it's a little verbose. I would like to see code like options.merge!(SharedGeneratorOptions.options) factored out into mixlib-cli so you could just write include_options(SharedGeneratorOptions) and have mixlib-cli work out the other details, though.

[martinisoft]

I have updated the original request text and waiting on Travis to build.

Regarding the self.included suggestion, I personally prefer going the composition method that @danielsdeleo suggests instead.

[sethvargo]

Fair enough

[sethvargo]

Travis is happy. 👍

[danielsdeleo]

LGTM. @opscode/client-eng Look good to you?

[sersut]

How about naming the option generator?

We can make this option more intelligent in the future to just get a simple recipe in addition to a cookbook as well.

[sersut]

Also would be great if you can leave a note in CHANGELOG.md. Please drop in your github id there as well 😄

[sethvargo]

@serdar I answered that in my comment above:

How about naming the option generator?

I think generator is a bad name because it refers to the internals. I would expect generator to change the underlying technology, not the layout.

[sethvargo]

@martinisoft I think @serdar was talking to you here:

Also would be great if you can leave a note in CHANGELOG.md. Please drop in your github id there as well 😄

[danielsdeleo]

@sersut I'm happy with --generator-cookbook -- it's verbose, but that's why we have a short option. --generator-cookbook will be nice and clear in the help output.

[sersut]

So what I'm saying is we can change this feature in the future to get either a cookbook or a straight recipe. This would make it easier to write a generator since one doesn't need to adhere to cookbook file layout.

If we name this --generator-cookbook now, we will need to have a --generator-recipe when we implement the above functionality.

[sethvargo]

CLA Verified (840)

[sersut]

Convinced that the option should be called --generator-cookbook

[martinisoft]

@sersut updated the CHANGELOG, let me know if the explanation and formatting is sufficient.

[sersut]

Thanks a lot @martinisoft. Awesome job!!