[SHACK-141] Lookup cookbooks in repo for remote target execution #82
Conversation
| cookbook_name = path_or_name | ||
| # Second, is there a cookbook in their local repo that matches? | ||
| require "chef/cookbook_loader" | ||
| cookbook_repo_path ||= ChefConfig::Config[:cookbook_path] |
There was a problem hiding this comment.
We allow users to specify --chef-repo-path but we lookup cookbook_path defaults from the ChefConfig - thats a weird inconsistency
| # Now that we are leveraging Chef locally we want to perform some initial setup of it | ||
| def configure_chef | ||
| ChefConfig.logger = ChefWorkstation::Log | ||
| ChefConfig::WorkstationConfigLoader.new(nil).load |
There was a problem hiding this comment.
Check and make sure that if we don't have a knife.rb or chef config this doesn't blow up
There was a problem hiding this comment.
Done - did not blow up
| @@ -63,8 +68,13 @@ class Converge < ChefWorkstation::Command::Base | |||
| :boolean => true, | |||
| :default => Config.connection.winrm.ssl_verify | |||
|
|
|||
| option :chef_repo_path, | |||
There was a problem hiding this comment.
We should also allow them to specify this in the ~/.chef-workstation/config.toml
Code comments and help text should explain our lookup logic.
…or default to using Chef Workstation config
f6a8e49 to
2a15dbd
Compare
| @@ -90,7 +93,11 @@ commands: | |||
| the name of the user you wanted to create. | |||
| <RECIPE> The recipe to converge. This can be provided as one of: | |||
| 1. Full path to a recipe file | |||
|
|
|||
| 2. Cookbook name. First we check the working directory for this | |||
There was a problem hiding this comment.
Here and below, s/repo/repository - we're trying to keep output very clear and explicit without abbreviations or shorthand.
| 2. Cookbook name. First we check the working directory for this | ||
| cookbook, then we check in the chef repo path. If a cookbook | ||
| is found we run the default recipe. | ||
| 3. Cookbook and recipe name. Same as 2 but we run the specified |
There was a problem hiding this comment.
Suggest: This behaves similarly to 'cookbook name' above, but it allows you to further specify which recipe to use from the cookbook.
| @@ -100,6 +107,8 @@ commands: | |||
| Use --ssl-no-verify when using SSL for WinRM and | |||
| the remote host is using a self-signed certificate. | |||
| Current default: %1 | |||
| cookbook_repo_paths: | |||
| desc: "Location of the Chef repo containing cookbooks to execute on targets. Comma seperated list of paths" | |||
There was a problem hiding this comment.
Suggest: A comma separated list of cookbook repository paths.
| @@ -237,11 +246,35 @@ errors: | |||
| Property '%1' did not match the 'key=value' syntax required | |||
|
|
|||
| CHEFVAL004: | | |||
| Please provide a recipe in the form 'path/to/recipe/file', 'cookbook_name', | |||
| or 'cookbook_name::recipe_name'. | |||
| Please provide a recipe in the form 'path/to/recipe/file.rb', | |||
There was a problem hiding this comment.
Do we require the '.rb'? If not, maybe file[.rb] in the usual CLI convention for optional things?
There was a problem hiding this comment.
We do require the .rb (full path required). We early on thought about trying to find a default .chef extension but I think that was trying to think too far ahead without considering the full story of that.
|
|
||
| You provided '%1'. | ||
|
|
||
| CHEFVAL005: | | ||
| Cookbook provided could not be loaded. Ensure it contains a valid |
There was a problem hiding this comment.
"The cookbook provided..."
|
|
||
| You provided '%1'. | ||
|
|
||
| CHEFVAL005: | | ||
| Cookbook provided could not be loaded. Ensure it contains a valid | ||
| 'metadata.rb' or a single recipe file named 'recipe.rb' only. |
There was a problem hiding this comment.
If we don't need to give them a choice, let's not. I'm more in favor of concise actionable messages than exhaustive ones - if they want more info, they can look up the error online which will give us a good place to expand on details.
In a future iteration, we could probably inspect the location and determine specifically the thing that is missing?
There was a problem hiding this comment.
In a future iteration, we could probably inspect the location and determine specifically the thing that is missing?
Yeah, I'm a fan of this
| Cookbook path is '%1'. | ||
|
|
||
| CHEFVAL008: | | ||
| Could not find a recipe named '%2' in your cookbook. Please specify |
There was a problem hiding this comment.
Suggest:
There is no recipe named '%2' in the cookbook '%4', which I found in %1.
Please include the name of the recipe
you wish to converge on the remote target.
These are the available recipes in %4:
%3
| %2 | ||
|
|
||
| CHEFVAL007: | | ||
| Cookbook '%2' did not contain a default recipe and you did not specify |
There was a problem hiding this comment.
Suggest:
There is no default recipe in cookbook '%2'. Please provide the name of the recipe to run, for example:
my_cookbook::some_recipe```
Better if you can use the actual cookbook name they gave instead of 'my_cookbook'
|
|
||
| module ChefWorkstation | ||
| module Command | ||
| class Target | ||
| class Converge < ChefWorkstation::Command::Base | ||
| T = ChefWorkstation::Text.commands.target.converge | ||
| TS = ChefWorkstation::Text.status | ||
| Config = ChefWorkstation::Config |
There was a problem hiding this comment.
This is a pattern that we follow throughout the project. It reduces the visual noise of Name::Space::Goes::Here for frequently referenced modules in a class. Is there anything unhealthy about doing that?
There was a problem hiding this comment.
I got rid of this specific instance because it overlaps with config (lower case c) exposed by mixlib-cli. It was weird to me having both config and Config
I actually think long term we should rename the method mixlib-cli exposes to something better, like cli_options or parsed_options.
Code comments and help text should explain our lookup logic.