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

[SHACK-141] Lookup cookbooks in repo for remote target execution #82

Merged
merged 4 commits into from
Apr 20, 2018
Merged

[SHACK-141] Lookup cookbooks in repo for remote target execution #82

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d165bf7
[SHACK-141] Lookup cookbooks in repo for remote target execution
tyler-ball Apr 12, 2018
2aef8ae
Redirecting Chef log to our log device so we do not get Chef logs app…
tyler-ball Apr 17, 2018
2a15dbd
Allowing user to specify cookbook_repo_paths as CLI arg, config file …
tyler-ball Apr 18, 2018
aa08031
Updating output strings we show users to be more clear
tyler-ball Apr 20, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 36 additions & 3 deletions components/chef-workstation/i18n/en.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,9 @@ commands:
<RECIPE>. For example:

chef target converge myec2node path/to/cookbook/recipe.rb
chef target converge myec2node path/to/cookbook
chef target converge myec2node cookbook_name
chef target converge myec2node cookbook_name::recipe_name

ARGUMENTS:
<TARGET> The host or IP address to converge. Can also be an SSH or WinRM URL
Expand All @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

Here and below, s/repo/repository - we're trying to keep output very clear and explicit without abbreviations or shorthand.

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
Copy link
Member

Choose a reason for hiding this comment

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

Suggest: This behaves similarly to 'cookbook name' above, but it allows you to further specify which recipe to use from the cookbook.

recipe instead of the default recipe.
root_description: "Whether to use root permissions on the target. Defaults to true."
identity_file: "SSH identity file to use when connecting."
ssl:
Expand All @@ -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"
Copy link
Member

Choose a reason for hiding this comment

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

Suggest: A comma separated list of cookbook repository paths.



# Status updates shared across commands.
Expand Down Expand Up @@ -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',
Copy link
Member

Choose a reason for hiding this comment

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

Do we require the '.rb'? If not, maybe file[.rb] in the usual CLI convention for optional things?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

'path/to/cookbook', 'cookbook_name' or 'cookbook_name::recipe_name'.

You provided '%1'.

CHEFVAL005: |
Cookbook provided could not be loaded. Ensure it contains a valid
Copy link
Member

Choose a reason for hiding this comment

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

"The cookbook provided..."

'metadata.rb' or a single recipe file named 'recipe.rb' only.
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

CHEFVAL006: |
Cookbook '%1' could not be found in any of the following directorys

%2

CHEFVAL007: |
Cookbook '%2' did not contain a default recipe and you did not specify
Copy link
Member

@marcparadise marcparadise Apr 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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'

a named recipe to run. Specify a valid recipe path or name.

Cookbook path is '%1'.

CHEFVAL008: |
Could not find a recipe named '%2' in your cookbook. Please specify
Copy link
Member

@marcparadise marcparadise Apr 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

a valid recipe name.

Cookbook path is '%1'.
Available recipes are: %3

# General errors/unknown errors are handled with CHEFINT
CHEFINT001: |
An unexpected error has occurred:
Expand Down
3 changes: 1 addition & 2 deletions components/chef-workstation/lib/chef-workstation/cli.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,7 @@ def setup_cli
Config.create_default_config_file
end
Config.load
ChefWorkstation::Log.setup(Config.log.location)
Log.level = Config.log.level.to_sym
ChefWorkstation::Log.setup(Config.log.location, Config.log.level.to_sym)
ChefWorkstation::Log.info("Initialized logger")
end

Expand Down
48 changes: 34 additions & 14 deletions components/chef-workstation/lib/chef-workstation/command/target/converge.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,14 +23,17 @@
require "chef-workstation/ui/terminal"
require "chef-workstation/log"
require "chef-workstation/config"
require "chef-workstation/recipe_lookup"
require "chef-config/config"
require "chef-config/logger"
require "chef/log"

module ChefWorkstation
module Command
class Target
class Converge < ChefWorkstation::Command::Base
T = ChefWorkstation::Text.commands.target.converge
TS = ChefWorkstation::Text.status
Config = ChefWorkstation::Config
Copy link
Member

Choose a reason for hiding this comment

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

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?

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


option :root,
:long => "--[no-]root",
Expand All @@ -52,19 +55,26 @@ class Converge < ChefWorkstation::Command::Base
option :ssl,
:long => "--[no-]ssl",
:short => "-s",
:description => T.ssl.desc(Config.connection.winrm.ssl),
:description => T.ssl.desc(ChefWorkstation::Config.connection.winrm.ssl),
:boolean => true,
:default => Config.connection.winrm.ssl
:default => ChefWorkstation::Config.connection.winrm.ssl

option :ssl_verify,
:long => "--[no-]ssl-verify",
:short => "-s",
:description => T.ssl.verify_desc(Config.connection.winrm.ssl_verify),
:description => T.ssl.verify_desc(ChefWorkstation::Config.connection.winrm.ssl_verify),
:boolean => true,
:default => Config.connection.winrm.ssl_verify
:default => ChefWorkstation::Config.connection.winrm.ssl_verify

option :cookbook_repo_paths,
:long => "--cookbook-repo-paths PATH",
:description => T.cookbook_repo_paths.desc,
:default => ChefWorkstation::Config.chef.cookbook_repo_paths,
:proc => Proc.new { |paths| paths.split(",") }

def run(params)
validate_params(cli_arguments)
configure_chef

target = cli_arguments.shift

Expand Down Expand Up @@ -109,6 +119,15 @@ def validate_params(params)
end
end

# Now that we are leveraging Chef locally we want to perform some initial setup of it
def configure_chef
ChefConfig.logger = ChefWorkstation::Log
# Setting the config isn't enough, we need to ensure the logger is initialized
# or automatic initialization will still go to stdout
Chef::Log.init(ChefWorkstation::Log)
Chef::Log.level = ChefWorkstation::Log.level
end

def format_properties(string_props)
properties = {}
string_props.each do |a|
Expand Down Expand Up @@ -147,21 +166,22 @@ def parse_converge_args(converge_args, cli_arguments)
if recipe_strategy?(cli_arguments)
recipe_specifier = cli_arguments.shift
ChefWorkstation::Log.debug("Beginning to look for recipe specified as #{recipe_specifier}")

# First, we check to see if the user has specified the full path (absolute or relative)
# to a file. If they have, we assume that is a recipe they want to execute.
if File.file?(recipe_specifier)
ChefWorkstation::Log.debug("#{recipe_specifier} is a valid path to a recipe")
converge_args[:recipe_path] = recipe_specifier
spinner_msg = TS.converge.converging_recipe(recipe_specifier)
recipe_path = recipe_specifier
else
raise "Cannot specify anything besides full path yet"
rl = RecipeLookup.new(config[:cookbook_repo_paths])
cookbook_path_or_name, optional_recipe_name = rl.split(recipe_specifier)
cookbook = rl.load_cookbook(cookbook_path_or_name)
recipe_path = rl.find_recipe(cookbook, optional_recipe_name)
end
converge_args[:recipe_path] = recipe_path
spinner_msg = TS.converge.converging_recipe(recipe_specifier)
else
resource = converge_args[:resource_type] = cli_arguments.shift
resource_name = converge_args[:resource_name] = cli_arguments.shift
converge_args[:resource_type] = cli_arguments.shift
converge_args[:resource_name] = cli_arguments.shift
converge_args[:properties] = format_properties(cli_arguments)
full_rs_name = "#{resource}[#{resource_name}]"
full_rs_name = "#{converge_args[:resource_type]}[#{converge_args[:resource_name]}]"
ChefWorkstation::Log.debug("Converging resource #{full_rs_name} on target")
spinner_msg = TS.converge.converging_resource(full_rs_name)
end
Expand Down
79 changes: 47 additions & 32 deletions components/chef-workstation/lib/chef-workstation/config.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,46 +1,24 @@
require "mixlib/config"
require "fileutils"
require "pathname"
require "chef-config/config"
require "chef-config/workstation_config_loader"

module ChefWorkstation
class Config
WS_BASE_PATH = File.join(Dir.home, ".chef-workstation/")

extend Mixlib::Config

config_strict_mode true

# When working on chef-workstation itself,
# developers should set telemetry.dev to true
# in their local configuration to ensure that dev usage
# doesn't skew customer telemetry.
config_context :telemetry do
default(:dev, false)
end

config_context :log do
default(:level, "warn")
default(:location, File.join(WS_BASE_PATH, "logs/default.log"))
end

config_context :cache do
default(:path, File.join(WS_BASE_PATH, "cache"))
end

config_context :connection do
config_context :winrm do
default(:ssl, false)
default(:ssl_verify, true)
end
end

config_context :dev do
default(:spinner, "TTY::Spinner")
end

class << self
@custom_location = nil

# Ensure when we extend Mixlib::Config that we load
# up the workstation config since we will need that
# to converge later
def initialize_mixlib_config
super
ChefConfig::WorkstationConfigLoader.new(nil).load
end

def custom_location(path)
@custom_location = path
raise "No config file located at #{path}" unless exist?
Expand Down Expand Up @@ -84,5 +62,42 @@ def reset
super
end
end

extend Mixlib::Config

config_strict_mode true

# When working on chef-workstation itself,
# developers should set telemetry.dev to true
# in their local configuration to ensure that dev usage
# doesn't skew customer telemetry.
config_context :telemetry do
default(:dev, false)
end

config_context :log do
default(:level, "warn")
default(:location, File.join(WS_BASE_PATH, "logs/default.log"))
end

config_context :cache do
default(:path, File.join(WS_BASE_PATH, "cache"))
end

config_context :connection do
config_context :winrm do
default(:ssl, false)
default(:ssl_verify, true)
end
end

config_context :dev do
default(:spinner, "TTY::Spinner")
end

config_context :chef do
default(:cookbook_repo_paths, ChefConfig::Config[:cookbook_path])
end

end
end
3 changes: 3 additions & 0 deletions components/chef-workstation/lib/chef-workstation/errors/ccr_failure_mapper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,9 @@ def exception_args_from_cause
when /.*Chef::Exceptions::ValidationFailed:\s+(.*)/
# Invalid resource property value
["CHEFCCR004", $1]
when /.*NameError: undefined local variable or method `(.+)' for cookbook.+/
# Invalid resource type in most cases
["CHEFCCR005", $1]
when /.*NoMethodError: undefined method `(.+)' for cookbook.+/
# Invalid resource type in most cases
["CHEFCCR005", $1]
Expand Down
3 changes: 2 additions & 1 deletion components/chef-workstation/lib/chef-workstation/log.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ module ChefWorkstation
class Log
extend Mixlib::Log

def self.setup(location, log_level = "warn")
def self.setup(location, log_level)
@location = location
if location.is_a?(String)
if location.casecmp("stdout") == 0
Expand All @@ -14,6 +14,7 @@ def self.setup(location, log_level = "warn")
end
end
init(location)
Log.level = log_level
end

def self.location
Expand Down
Loading