From 841a65c3c497cf3acd0fe3293d770841ef077707 Mon Sep 17 00:00:00 2001 From: Hunter Haugen Date: Wed, 21 Feb 2018 16:25:36 -0800 Subject: [PATCH 01/24] Add --template-ref argument for upstream template repo tags I rolled the url/refs together internally into a single "uri" that should be considered the template target and broken apart with the URI & decode methods when needed separately. - The decision code for obtaining the uri/ref has been centralized into the `PDK::Util` functions as discussed on the PR comments on github. - The template-ref value in the metadata is for pdk update reference, not for template targetting. - The default ref is to be used when no fragment is available in the URI. - The `PDK::Util.template_uri()` takes options hash for the opts.fetch in case users pass --arguments, but is a class method. I don't really like this, but it's probably needed to fulfill the requirements discussed in the PR. - The `file:///...` urls are treated as remote paths, but absolute paths such as `c:/...` or `/...` are treated as local paths and use directly. This behavior was pre-existing. - The work tree for the templates directory is only reset newly-cloned directorys, and should not be touched in the above-mentioned local-path directories. - Handling file paths with #'s in the name is overly complex due to the way URI encoding and URI fragments work. I just error in the case of #'s - The Addressable::URI library is the most-compatible URI handler, so I used that. - SCP-like urls of user@host:/path are not URI RFC-compatible, so I helpfully parse them into URIs and print a warning. (The ambiguous case is `user@host:<0-65535>/path` in which I interpret this as a URI instead of an SCP url) - I removed the --template-url --template-ref arguments from most of the `pdk new *` generators as they are not used to specify or change templates and actually create edge cases by being used to do so. The arguments should only be specified during a pdk convert or a `pdk new module` - Most of the spec fixes only have to do with shuffling a few stubs around and lining up string/addressable::uri expectations. - One awkward thing I noticed is that the spec_helper.rb on line 33 loads the answers by requiring pdk/cli and it happens before any stubs are placed so polluted answers.json on my machine is picked up instead of stubs. This should be documented probably as I lost a few hours to it. - Windows paths most start with either a `file:///:/...` scheme or `/:/` leading-slash to be URI-compatible. I represent paths that lack the `file://` scheme as `/:/` internally and strip the leading / when used for commands or serialization in metadata.json. --- lib/pdk/cli.rb | 6 +- lib/pdk/cli/convert.rb | 1 + lib/pdk/cli/module/generate.rb | 1 + lib/pdk/cli/new/class.rb | 2 - lib/pdk/cli/new/defined_type.rb | 2 - lib/pdk/cli/new/module.rb | 1 + lib/pdk/cli/new/provider.rb | 2 - lib/pdk/cli/new/task.rb | 1 - lib/pdk/generate/module.rb | 20 +- lib/pdk/generate/puppet_object.rb | 14 +- lib/pdk/module/convert.rb | 6 +- lib/pdk/module/templatedir.rb | 64 ++-- lib/pdk/module/update.rb | 23 +- lib/pdk/util.rb | 178 ++++++++--- lib/pdk/util/git.rb | 28 +- spec/spec_helper_acceptance.rb | 2 +- spec/unit/pdk/cli/convert_spec.rb | 8 + spec/unit/pdk/cli/new/class_spec.rb | 11 - spec/unit/pdk/cli/new/defined_type_spec.rb | 10 - spec/unit/pdk/cli/new/module_spec.rb | 10 + spec/unit/pdk/cli/new/task_spec.rb | 10 - spec/unit/pdk/generate/module_spec.rb | 33 +- spec/unit/pdk/generate/puppet_object_spec.rb | 10 +- spec/unit/pdk/module/convert_spec.rb | 1 + spec/unit/pdk/module/template_dir_spec.rb | 17 +- spec/unit/pdk/module/update_spec.rb | 17 +- spec/unit/pdk/util/git_spec.rb | 2 +- spec/unit/pdk/util_spec.rb | 310 +++++++++++++++---- 28 files changed, 565 insertions(+), 225 deletions(-) diff --git a/lib/pdk/cli.rb b/lib/pdk/cli.rb index 3103e1bba..70cd160c5 100644 --- a/lib/pdk/cli.rb +++ b/lib/pdk/cli.rb @@ -39,11 +39,15 @@ def self.run(args) end def self.template_url_option(dsl) - desc = _('Specifies the URL to the template to use when creating new modules or classes. (default: %{default_url})') % { default_url: PDK::Util.default_template_url } + desc = _('Specifies the URL to the template to use when creating new modules or classes. (default: %{default_url})') % { default_url: PDK::Util.default_template_uri } dsl.option nil, 'template-url', desc, argument: :required end + def self.template_ref_option(dsl) + dsl.option nil, 'template-ref', _('Specifies the template git branch or tag to use when creating new modules or classes.'), argument: :required + end + def self.skip_interview_option(dsl) dsl.option nil, 'skip-interview', _('When specified, skips interactive querying of metadata.') end diff --git a/lib/pdk/cli/convert.rb b/lib/pdk/cli/convert.rb index 206bcf953..72127101c 100644 --- a/lib/pdk/cli/convert.rb +++ b/lib/pdk/cli/convert.rb @@ -7,6 +7,7 @@ module PDK::CLI summary _('Convert an existing module to be compatible with the PDK.') PDK::CLI.template_url_option(self) + PDK::CLI.template_ref_option(self) PDK::CLI.skip_interview_option(self) PDK::CLI.full_interview_option(self) flag nil, :noop, _('Do not convert the module, just output what would be done.') diff --git a/lib/pdk/cli/module/generate.rb b/lib/pdk/cli/module/generate.rb index b466be6c9..afd3232d3 100644 --- a/lib/pdk/cli/module/generate.rb +++ b/lib/pdk/cli/module/generate.rb @@ -7,6 +7,7 @@ module PDK::CLI summary _('This command is now \'pdk new module\'.') PDK::CLI.template_url_option(self) + PDK::CLI.template_ref_option(self) PDK::CLI.skip_interview_option(self) run do |opts, args, _cmd| diff --git a/lib/pdk/cli/new/class.rb b/lib/pdk/cli/new/class.rb index ff51e93e2..d003f1505 100644 --- a/lib/pdk/cli/new/class.rb +++ b/lib/pdk/cli/new/class.rb @@ -4,8 +4,6 @@ module PDK::CLI usage _('class [options] ') summary _('Create a new class named using given options') - PDK::CLI.template_url_option(self) - run do |opts, args, _cmd| require 'pdk/generate/puppet_class' diff --git a/lib/pdk/cli/new/defined_type.rb b/lib/pdk/cli/new/defined_type.rb index 7df199362..fc48dc642 100644 --- a/lib/pdk/cli/new/defined_type.rb +++ b/lib/pdk/cli/new/defined_type.rb @@ -4,8 +4,6 @@ module PDK::CLI usage _('defined_type [options] ') summary _('Create a new defined type named using given options') - PDK::CLI.template_url_option(self) - run do |opts, args, _cmd| PDK::CLI::Util.ensure_in_module!( message: _('Defined types can only be created from inside a valid module directory.'), diff --git a/lib/pdk/cli/new/module.rb b/lib/pdk/cli/new/module.rb index 812bd6dfa..a0bb7d4e1 100644 --- a/lib/pdk/cli/new/module.rb +++ b/lib/pdk/cli/new/module.rb @@ -5,6 +5,7 @@ module PDK::CLI summary _('Create a new module named [module_name] using given options') PDK::CLI.template_url_option(self) + PDK::CLI.template_ref_option(self) PDK::CLI.skip_interview_option(self) PDK::CLI.full_interview_option(self) diff --git a/lib/pdk/cli/new/provider.rb b/lib/pdk/cli/new/provider.rb index db5c8d3ff..b89e836a0 100644 --- a/lib/pdk/cli/new/provider.rb +++ b/lib/pdk/cli/new/provider.rb @@ -4,8 +4,6 @@ module PDK::CLI usage _('provider [options] ') summary _('[experimental] Create a new ruby provider named using given options') - PDK::CLI.template_url_option(self) - run do |opts, args, _cmd| PDK::CLI::Util.ensure_in_module! diff --git a/lib/pdk/cli/new/task.rb b/lib/pdk/cli/new/task.rb index 78d23a0c5..8e98dfacc 100644 --- a/lib/pdk/cli/new/task.rb +++ b/lib/pdk/cli/new/task.rb @@ -4,7 +4,6 @@ module PDK::CLI usage _('task [options] ') summary _('Create a new task named using given options') - PDK::CLI.template_url_option(self) option nil, :description, _('A short description of the purpose of the task'), argument: :required run do |opts, args, _cmd| diff --git a/lib/pdk/generate/module.rb b/lib/pdk/generate/module.rb index f0113f188..bcf62c7d7 100644 --- a/lib/pdk/generate/module.rb +++ b/lib/pdk/generate/module.rb @@ -54,10 +54,10 @@ def self.invoke(opts = {}) prepare_module_directory(temp_target_dir) - template_url = opts.fetch(:'template-url', PDK::Util.default_template_url) + template_uri = PDK::Util.template_uri(opts) begin - PDK::Module::TemplateDir.new(template_url, metadata.data, true) do |templates| + PDK::Module::TemplateDir.new(template_uri, metadata.data, true) do |templates| templates.render do |file_path, file_content| file = Pathname.new(temp_target_dir) + file_path file.dirname.mkpath @@ -74,21 +74,23 @@ def self.invoke(opts = {}) raise PDK::CLI::ExitWithError, e end - if template_url == PDK::Util.puppetlabs_template_url - # If the user specifies our template via the command line, remove the - # saved template-url answer. + # Only update the answers files after metadata has been written. + if template_uri == PDK::Util.default_template_uri + # If the user specifies our default template url via the command + # line, remove the saved template-url answer so that the template_uri + # resolution can find new default URLs in the future. PDK.answers.update!('template-url' => nil) if opts.key?(:'template-url') else - # Save the template-url answer if the module was generated using - # a template other than ours. - PDK.answers.update!('template-url' => template_url) + # Save the template-url + template-ref answers if the module was + # generated using a template/reference other than ours. + PDK.answers.update!('template-url' => template_uri) end begin if FileUtils.mv(temp_target_dir, target_dir) Dir.chdir(target_dir) { PDK::Util::Bundler.ensure_bundle! } unless opts[:'skip-bundle-install'] - PDK.logger.info(_('Module \'%{name}\' generated at path \'%{path}\', from template \'%{template_url}\'.') % { name: opts[:module_name], path: target_dir, template_url: template_url }) + PDK.logger.info _('Module \'%{name}\' generated at path \'%{path}\', from template \'%{url}\'.') % { name: opts[:module_name], path: target_dir, url: PDK::Util.template_url(template_uri) } PDK.logger.info(_('In your module directory, add classes with the \'pdk new class\' command.')) end rescue Errno::EACCES => e diff --git a/lib/pdk/generate/puppet_object.rb b/lib/pdk/generate/puppet_object.rb index 5020d0d84..a06948cfc 100644 --- a/lib/pdk/generate/puppet_object.rb +++ b/lib/pdk/generate/puppet_object.rb @@ -29,7 +29,7 @@ class PuppetObject # @param options [Hash{Symbol => Object}] # # @api public - def initialize(module_dir, object_name, options = {}) + def initialize(module_dir, object_name, options) @module_dir = module_dir @options = options @object_name = object_name @@ -205,12 +205,12 @@ def write_file(dest_path) # @api private def with_templates templates.each do |template| - if template[:url].nil? - PDK.logger.debug(_('No %{dir_type} template specified; trying next template directory.') % { dir_type: template[:type] }) + if template[:uri].nil? + PDK.logger.debug(_('No %{dir_type} template found; trying next template directory.') % { dir_type: template[:type] }) next end - PDK::Module::TemplateDir.new(template[:url]) do |template_dir| + PDK::Module::TemplateDir.new(template[:uri]) do |template_dir| template_paths = template_dir.object_template_for(object_type) if template_paths @@ -250,11 +250,7 @@ def with_templates # # @api private def templates - @templates ||= [ - { type: 'CLI', url: @options[:'template-url'], allow_fallback: false }, - { type: 'metadata', url: module_metadata.data['template-url'], allow_fallback: true }, - { type: 'default', url: PDK::Util.default_template_url, allow_fallback: false }, - ] + @templates ||= PDK::Util.templates(@options) end # Retrieves the name of the module (without the forge username) from the diff --git a/lib/pdk/module/convert.rb b/lib/pdk/module/convert.rb index c9a10ab71..6db33d635 100644 --- a/lib/pdk/module/convert.rb +++ b/lib/pdk/module/convert.rb @@ -68,7 +68,7 @@ def needs_bundle_update? def stage_changes! metadata_path = 'metadata.json' - PDK::Module::TemplateDir.new(template_url, nil, false) do |templates| + PDK::Module::TemplateDir.new(template_uri, nil, false) do |templates| new_metadata = update_metadata(metadata_path, templates.metadata) templates.module_metadata = new_metadata.data unless new_metadata.nil? @@ -102,8 +102,8 @@ def update_manager @update_manager ||= PDK::Module::UpdateManager.new end - def template_url - @template_url ||= options.fetch(:'template-url', PDK::Util.default_template_url) + def template_uri + @template_uri ||= PDK::Util.template_uri(options) end def update_metadata(metadata_path, template_metadata) diff --git a/lib/pdk/module/templatedir.rb b/lib/pdk/module/templatedir.rb index 513ddfc6f..23f1001e3 100644 --- a/lib/pdk/module/templatedir.rb +++ b/lib/pdk/module/templatedir.rb @@ -16,8 +16,8 @@ class TemplateDir # The template directory is only guaranteed to be available on disk # within the scope of the block passed to this method. # - # @param path_or_url [String] The path to a directory to use as the - # template or a URL to a git repository. + # @param uri [Addressable::URI] The path to a directory to use as the + # template or a URI to a git repository. # @param module_metadata [Hash] A Hash containing the module metadata. # Defaults to an empty Hash. # @yieldparam self [PDK::Module::TemplateDir] The initialised object with @@ -37,17 +37,28 @@ class TemplateDir # @raise [ArgumentError] (see #validate_module_template!) # # @api public - def initialize(path_or_url, module_metadata = {}, init = false) + def initialize(uri, module_metadata = {}, init = false) unless block_given? raise ArgumentError, _('%{class_name} must be initialized with a block.') % { class_name: self.class.name } end + unless uri.is_a? Addressable::URI + raise ArgumentError, _('PDK::Module::TemplateDir.new must be initialized with an Addressable::URI, got a %{uri_type}') % { uri_type: uri.class } + end - if PDK::Util::Git.repo?(path_or_url) - @path = self.class.clone_template_repo(path_or_url) - @repo = path_or_url + if PDK::Util::Git.repo?(PDK::Util.template_url(uri)) + # This is either a bare local repo or a remote. either way it needs cloning. + @path = self.class.clone_template_repo(uri) + temp_dir_clone = true else - @path = path_or_url + # if it is a local path & non-bare repo then we can use it directly. + # Still have to check the branch. + @path = PDK::Util.template_path(uri) + # We don't do a checkout of local-path repos. There are lots of edge + # cases or user un-expectations. + # self.class.checkout_template_ref(@path, PDK::Util.template_ref(uri)) + PDK.logger.warn _("Repository '%{repo}' has a work-tree; skipping git reset.") % { repo: @path } end + @cloned_from = PDK::Util.deuri_path(uri.to_s) @init = init @moduleroot_dir = File.join(@path, 'moduleroot') @@ -64,7 +75,7 @@ def initialize(path_or_url, module_metadata = {}, init = false) ensure # If we cloned a git repo to get the template, remove the clone once # we're done with it. - if @repo + if temp_dir_clone FileUtils.remove_dir(@path) end end @@ -82,7 +93,7 @@ def metadata 'pdk-version' => PDK::Util::Version.version_string, } - result['template-url'] = @repo ? @repo : @path + result['template-url'] = @cloned_from ref_result = PDK::Util::Git.git('--git-dir', File.join(@path, '.git'), 'describe', '--all', '--long', '--always') result['template-ref'] = ref_result[:stdout].strip if ref_result[:exit_code].zero? @@ -118,11 +129,11 @@ def render else begin dest_content = PDK::TemplateFile.new(File.join(template_loc, template_file), configs: config, template_dir: self).render - rescue => e + rescue => error error_msg = _( "Failed to render template '%{template}'\n" \ '%{exception}: %{message}', - ) % { template: template_file, exception: e.class, message: e.message } + ) % { template: template_file, exception: error.class, message: error.message } raise PDK::CLI::FatalError, error_msg end end @@ -290,24 +301,18 @@ def read_config(loc) # @raise [PDK::CLI::FatalError] If reset HEAD of the cloned repo to desired ref. # # @api private - def self.clone_template_repo(origin_repo) + def self.clone_template_repo(uri) # @todo When switching this over to using rugged, cache the cloned # template repo in `%AppData%` or `$XDG_CACHE_DIR` and update before # use. temp_dir = PDK::Util.make_tmpdir_name('pdk-templates') - git_ref = (origin_repo == PDK::Util.default_template_url) ? PDK::Util.default_template_ref : 'origin/master' + origin_repo = PDK::Util.template_url(uri) + git_ref = PDK::Util.template_ref(uri) clone_result = PDK::Util::Git.git('clone', origin_repo, temp_dir) if clone_result[:exit_code].zero? - Dir.chdir(temp_dir) do - reset_result = PDK::Util::Git.git('reset', '--hard', git_ref) - unless reset_result[:exit_code].zero? - PDK.logger.error reset_result[:stdout] - PDK.logger.error reset_result[:stderr] - raise PDK::CLI::FatalError, _("Unable to set HEAD of git repository at '%{repo}' to ref:'%{ref}'.") % { repo: temp_dir, ref: git_ref } - end - end + checkout_template_ref(temp_dir, git_ref) else PDK.logger.error clone_result[:stdout] PDK.logger.error clone_result[:stderr] @@ -316,6 +321,23 @@ def self.clone_template_repo(origin_repo) PDK::Util.canonical_path(temp_dir) end + + # @api private + def self.checkout_template_ref(path, ref) + if PDK::Util::Git.work_dir_clean?(path) + Dir.chdir(path) do + full_ref = PDK::Util::Git.ls_remote(path, ref) + reset_result = PDK::Util::Git.git('reset', '--hard', full_ref) + return if reset_result[:exit_code].zero? + + PDK.logger.error reset_result[:stdout] + PDK.logger.error reset_result[:stderr] + raise PDK::CLI::FatalError, _("Unable to set HEAD of git repository at '%{repo}' to ref:'%{ref}'.") % { repo: path, ref: ref } + end + else + PDK.logger.warn _("Uncommitted changes found when attempting to set HEAD of git repository at '%{repo}' to ref '%{ref}'; skipping git reset.") % { repo: path, ref: ref } + end + end end end end diff --git a/lib/pdk/module/update.rb b/lib/pdk/module/update.rb index adb758c3d..746fddec6 100644 --- a/lib/pdk/module/update.rb +++ b/lib/pdk/module/update.rb @@ -50,8 +50,11 @@ def module_metadata raise PDK::CLI::ExitWithError, e.message end - def template_url - @template_url ||= module_metadata.data['template-url'] + # TODO: update should be able to use exsting url. uri fragments should be + # tracked if existing, but the template-ref in the metadata is not for + # tracking simply for referencing the last update target. + def metadata_template_uri + @metadata_template_uri ||= Addressable::URI.parse(module_metadata.data['template-url']) end def current_version @@ -65,6 +68,8 @@ def new_version private def current_template_version + # TODO: Should the current/new versions be queried here or should that + # come from the util class? @current_template_version ||= module_metadata.data['template-ref'] end @@ -81,19 +86,19 @@ def describe_ref_to_s(describe_ref) end def new_template_version - PDK::Util.default_template_ref + metadata_template_uri.fragment || PDK::Util.default_template_ref end - def fetch_remote_version(version) - return version unless version.include?('/') + def fetch_remote_version(template_ref) + return template_ref unless current_template_version.is_a?(String) + return template_ref if template_ref == PDK::TEMPLATE_REF - branch = version.partition('/').last sha_length = GIT_DESCRIBE_PATTERN.match(current_template_version)[:sha].length - 1 - "#{branch}@#{PDK::Util::Git.ls_remote(template_url, "refs/heads/#{branch}")[0..sha_length]}" + "#{template_ref}@#{PDK::Util::Git.ls_remote(PDK::Util.template_url(metadata_template_uri), template_ref)[0..sha_length]}" end def update_message - format_string = if template_url == PDK::Util.puppetlabs_template_url + format_string = if metadata_template_uri == PDK::Util.default_template_uri _('Updating %{module_name} using the default template, from %{current_version} to %{new_version}') else _('Updating %{module_name} using the template at %{template_url}, from %{current_version} to %{new_version}') @@ -101,7 +106,7 @@ def update_message format_string % { module_name: module_metadata.data['name'], - template_url: template_url, + template_url: PDK::Util.template_url(metadata_template_uri), current_version: current_version, new_version: new_version, } diff --git a/lib/pdk/util.rb b/lib/pdk/util.rb index 705f06fbb..bf5ae23db 100644 --- a/lib/pdk/util.rb +++ b/lib/pdk/util.rb @@ -197,62 +197,164 @@ def targets_relative_to_pwd(targets) end module_function :targets_relative_to_pwd - def default_template_url - answer_file_url = PDK.answers['template-url'] - - return puppetlabs_template_url if answer_file_url.nil? + # @returns String + def template_url(uri) + if uri.is_a?(Addressable::URI) && uri.fragment + deuri_path(uri.to_s.chomp('#' + uri.fragment)) + else + deuri_path(uri.to_s) + end + end + module_function :template_url + # @returns String + def template_ref(uri) + if uri.fragment + uri.fragment + else + default_template_ref + end + end + module_function :template_ref + # @returns String + def template_path(uri) + deuri_path(uri.path) + end + module_function :template_path + # C:\... urls are not URI-safe. They should be /C:\... to be so. + # @returns String + def uri_path(path) + (Gem.win_platform? && path =~ %r{^[a-zA-Z][\|:]}) ? "/#{path}" : path + end + module_function :uri_path + # @returns String + def deuri_path(path) + (Gem.win_platform? && path =~ %r{^\/[a-zA-Z][\|:]}) ? path[1..-1] : path + end + module_function :deuri_path + + # @returns Addressable::URI + def template_uri(opts) + # 1. Get the four sources of URIs + # 2. Pick the first non-nil URI + # 3. Error if the URI is not a valid git repo (missing directory or http 404) + # 4. Leave updating answers/metadata to other code + # XXX Make it work before making it pretty. + found_template = templates(opts).find { |t| valid_template?(t) } + + raise PDK::CLI::FatalError, _('Unable to find a valid module template to use.') if found_template.nil? + found_template[:uri] + end + module_function :template_uri - # Ignore answer file template-url if the value is the old or new default. - return puppetlabs_template_url if answer_file_url == 'https://github.com/puppetlabs/pdk-module-template' - return puppetlabs_template_url if answer_file_url == puppetlabs_template_url + def valid_template?(template) + return false if template.nil? + return false if template[:uri].nil? - if File.directory?(answer_file_url) - # Instantiating a new TemplateDir object pointing to the directory - # will cause the directory contents to be validated, raising - # ArgumentError if it does not appear to be a valid template. - PDK::Module::TemplateDir.new(answer_file_url) {} - return answer_file_url + if template[:path] && File.directory?(template[:path]) + PDK::Module::TemplateDir.new(template[:uri]) {} + return true end + return true if PDK::Util::Git.repo?(template[:uri]) - raise ArgumentError unless PDK::Util::Git.repo?(answer_file_url) - - answer_file_url + false rescue ArgumentError - PDK.logger.warn( - _("Unable to access the previously used template '%{template}', using the default template instead.") % { - template: answer_file_url, - }, - ) - PDK.answers.update!('template-url' => nil) - return puppetlabs_template_url + return false end - module_function :default_template_url + module_function :valid_template? + + # @return [Array Object}>] an array of hashes. Each hash + # contains 3 keys: :type contains a String that describes the template + # directory, :url contains a String with the URL to the template + # directory, and :allow_fallback contains a Boolean that specifies if + # the lookup process should proceed to the next template directory if + # the template file is not in this template directory. + # + def templates(opts) + explicit_url = opts.fetch(:'template-url', nil) + explicit_ref = opts.fetch(:'template-ref', nil) - def puppetlabs_template_url - if package_install? - 'file://' + File.join(package_cachedir, 'pdk-templates.git') + if explicit_ref && explicit_url.nil? + raise PDK::CLI::FatalError, _('--template-ref requires --template-url to also be specified.') + end + if explicit_url && explicit_url.include?('#') + raise PDK::CLI::FatalError, _('--template-url may not be used to specify paths containing #\'s') + end + + # 1. Get the CLI, metadata (or answers if no metadata), and default URIs + # 2. Construct the hash + if explicit_url + # Valid URIs to avoid catching: + # - absolute local paths + # - have :'s in paths when preceeded by a slash + # - have only digits following the : and preceeding a / or end-of-string that is 0-65535 + # The last item is ambiguous in the case of scp/git paths vs. URI port + # numbers, but can be made unambiguous by making the form to + # ssh://git@github.com/1234/repo.git or + # ssh://git@github.com:1234/user/repo.git + scp_url_m = explicit_url.match(%r{\A(.*@[^/:]+):(.+)\z}) + if Pathname.new(explicit_url).relative? && scp_url_m + # "^git@..." is also malformed as it is missing a scheme + # "ssh://git@..." is correct. + check_url = Addressable::URI.parse(scp_url_m[1]) + scheme = 'ssh://' unless check_url.scheme + + numbers_m = scp_url_m[2].split('/')[0].match(%r{\A[0-9]+\z}) + if numbers_m && numbers_m[0].to_i < 65_536 + # consider it an explicit-port URI, even though it's ambiguous. + explicit_url = Addressable::URI.parse(scheme + scp_url_m[1] + ':' + scp_url_m[2]) + else + explicit_url = Addressable::URI.parse(scheme + scp_url_m[1] + '/' + scp_url_m[2]) + PDK.logger.warn(_('--template-url appears to be an SCP-style url; it will be converted to an RFC-compliant URI: %{uri}') % { uri: explicit_url }) + end + end + explicit_uri = Addressable::URI.parse(uri_path(explicit_url)) + explicit_uri.fragment = explicit_ref else - 'https://github.com/puppetlabs/pdk-templates' + explicit_uri = nil end + metadata_uri = if module_root && File.file?(File.join(module_root, 'metadata.json')) + Addressable::URI.parse(uri_path(module_metadata['template-url'])) + else + nil + end + answers_uri = if PDK.answers['template-url'] == 'https://github.com/puppetlabs/pdk-module-template' + # use the new github template-url if it is still the old one. + default_template_uri + elsif PDK.answers['template-url'] + Addressable::URI.parse(uri_path(PDK.answers['template-url'])) + else + nil + end + default_uri = default_template_uri + + ary = [] + ary << { type: _('--template-url'), uri: explicit_uri, allow_fallback: false } + ary << { type: _('metadata.json'), uri: metadata_uri, allow_fallback: true } if metadata_uri + ary << { type: _('PDK answers'), uri: answers_uri, allow_fallback: true } unless metadata_uri + ary << { type: _('default'), uri: default_uri, allow_fallback: false } + ary end - module_function :puppetlabs_template_url - - def default_template_ref - # TODO: This should respect a --template-ref option if we add that - return 'origin/master' if default_template_url != puppetlabs_template_url + module_function :templates - puppetlabs_template_ref + # @returns Addressable::URI + def default_template_uri + if package_install? + Addressable::URI.new(scheme: 'file', host: '', path: File.join(package_cachedir, 'pdk-templates.git')) + else + Addressable::URI.parse('https://github.com/puppetlabs/pdk-templates') + end end - module_function :default_template_ref + module_function :default_template_uri - def puppetlabs_template_ref + # @returns String + def default_template_ref if PDK::Util.development_mode? - 'origin/master' + 'master' else PDK::TEMPLATE_REF end end - module_function :puppetlabs_template_ref + module_function :default_template_ref # TO-DO: Refactor replacement of lib/pdk/module/build.rb:metadata to use this function instead def module_metadata diff --git a/lib/pdk/util/git.rb b/lib/pdk/util/git.rb index 4058d4790..8a4115bc1 100644 --- a/lib/pdk/util/git.rb +++ b/lib/pdk/util/git.rb @@ -39,7 +39,8 @@ def self.git_with_env(env, *args) end def self.repo?(maybe_repo) - return bare_repo?(maybe_repo) if File.directory?(maybe_repo) + # The behavior of file://... paths remote appears to be on purpose. + return bare_repo?(maybe_repo) if File.directory?(maybe_repo) # || (uri.scheme == 'file' && File.directory?(PDK::Util.template_path(uri))) remote_repo?(maybe_repo) end @@ -55,8 +56,27 @@ def self.remote_repo?(maybe_repo) git('ls-remote', '--exit-code', maybe_repo)[:exit_code].zero? end + def self.work_dir_clean?(repo) + raise PDK::CLI::ExitWithError, _('Unable to locate git work dir "%{workdir}') % { workdir: repo } unless File.directory?(repo) + raise PDK::CLI::ExitWithError, _('Unable to locate git dir "%{gitdir}') % { gitdir: repo } unless File.directory?(File.join(repo, '.git')) + + git('--work-tree', repo, '--git-dir', File.join(repo, '.git'), 'status', '--untracked-files=no', '--porcelain', repo)[:stdout].empty? + end + + def self.current_branch(repo) + env = { 'GIT_DIR' => repo } + rev_parse = git_with_env(env, 'rev-parse', '--abbrev-ref', 'HEAD') + + rev_parse[:stdout].strip + end + def self.ls_remote(repo, ref) - output = git('ls-remote', '--refs', repo, ref) + output = if File.directory?(repo) + # Appveyor doesn't like windows-path repository targets + git('--git-dir', File.join(repo, '.git'), 'ls-remote', '--refs', 'origin', ref) + else + git('ls-remote', '--refs', repo, ref) + end unless output[:exit_code].zero? PDK.logger.error output[:stdout] @@ -67,7 +87,9 @@ def self.ls_remote(repo, ref) end matching_refs = output[:stdout].split("\n").map { |r| r.split("\t") } - matching_refs.find { |_sha, remote_ref| remote_ref == ref }.first + matching_ref = matching_refs.find { |_sha, remote_ref| remote_ref == "refs/tags/#{ref}" || remote_ref == "refs/remotes/origin/#{ref}" || remote_ref == "refs/heads/#{ref}" } + raise PDK::CLI::ExitWithError, _('Unable to find a branch or tag named "%{ref}" in %{repo}') % { ref: ref, repo: repo } if matching_ref.nil? + matching_ref.first end end end diff --git a/spec/spec_helper_acceptance.rb b/spec/spec_helper_acceptance.rb index 632e336ef..8eade458d 100644 --- a/spec/spec_helper_acceptance.rb +++ b/spec/spec_helper_acceptance.rb @@ -60,7 +60,7 @@ def execute_script(script) RSpec.configure do |c| c.before(:suite) do RSpec.configuration.template_dir = Dir.mktmpdir - output, status = Open3.capture2e('git', 'clone', '--bare', PDK::Util.default_template_url, RSpec.configuration.template_dir) + output, status = Open3.capture2e('git', 'clone', '--bare', PDK::Util.default_template_uri, RSpec.configuration.template_dir) raise "Failed to cache module template: #{output}" unless status.success? tempdir = Dir.mktmpdir diff --git a/spec/unit/pdk/cli/convert_spec.rb b/spec/unit/pdk/cli/convert_spec.rb index 2810a2c49..3016b310f 100644 --- a/spec/unit/pdk/cli/convert_spec.rb +++ b/spec/unit/pdk/cli/convert_spec.rb @@ -34,6 +34,14 @@ end end + context 'and the --template-ref option has been passed' do + it 'invokes the converter with the user supplied template' do + expect(PDK::Module::Convert).to receive(:invoke).with(:'template-url' => anything, :'template-ref' => '1.0.0') + + PDK::CLI.run(['convert', '--template-url', anything, '--template-ref', '1.0.0']) + end + end + context 'and the --noop flag has been passed' do it 'passes the noop option through to the converter' do expect(PDK::Module::Convert).to receive(:invoke).with(noop: true) diff --git a/spec/unit/pdk/cli/new/class_spec.rb b/spec/unit/pdk/cli/new/class_spec.rb index 116f510ce..dc595b829 100644 --- a/spec/unit/pdk/cli/new/class_spec.rb +++ b/spec/unit/pdk/cli/new/class_spec.rb @@ -47,17 +47,6 @@ PDK::CLI.run(%w[new class test_class]) end - - context 'and a custom template URL' do - it 'generates the class from the custom template' do - expect(PDK::Generate::PuppetClass).to receive(:new) - .with(anything, 'test_class', :'template-url' => 'https://custom/template') - .and_return(generator) - expect(generator).to receive(:run) - - PDK::CLI.run(%w[new class test_class --template-url https://custom/template]) - end - end end end end diff --git a/spec/unit/pdk/cli/new/defined_type_spec.rb b/spec/unit/pdk/cli/new/defined_type_spec.rb index 47165b4a8..5da744b1e 100644 --- a/spec/unit/pdk/cli/new/defined_type_spec.rb +++ b/spec/unit/pdk/cli/new/defined_type_spec.rb @@ -64,16 +64,6 @@ PDK::CLI.run(%w[new defined_type test_define]) end - - context 'and a custom template URL' do - let(:generator_opts) { { :'template-url' => 'https://custom/template' } } - - it 'generates the defined type from the custom template' do - expect(generator_double).to receive(:run) - - PDK::CLI.run(%w[new defined_type test_define --template-url https://custom/template]) - end - end end end end diff --git a/spec/unit/pdk/cli/new/module_spec.rb b/spec/unit/pdk/cli/new/module_spec.rb index ee6212a21..0a8c741cb 100644 --- a/spec/unit/pdk/cli/new/module_spec.rb +++ b/spec/unit/pdk/cli/new/module_spec.rb @@ -48,6 +48,16 @@ end end + context 'and the template-ref option' do + let(:template_ref) { '1.0.0' } + + it 'passes the value of the template-ref option to PDK::Generate::Module.invoke' do + expect(PDK::Generate::Module).to receive(:invoke).with(hash_including(:'template-ref' => template_ref)) + expect(logger).to receive(:info).with("Creating new module: #{module_name}") + PDK::CLI.run(['new', 'module', '--template-ref', template_ref, module_name]) + end + end + context 'and the license option' do let(:license) { 'MIT' } diff --git a/spec/unit/pdk/cli/new/task_spec.rb b/spec/unit/pdk/cli/new/task_spec.rb index 82733b648..3d677ffaf 100644 --- a/spec/unit/pdk/cli/new/task_spec.rb +++ b/spec/unit/pdk/cli/new/task_spec.rb @@ -65,16 +65,6 @@ PDK::CLI.run(%w[new task test_task]) end - context 'and a custom template URL' do - let(:generator_opts) { { :'template-url' => 'https://custom/template' } } - - it 'generates the task from the custom template' do - expect(generator_double).to receive(:run) - - PDK::CLI.run(%w[new task test_task --template-url https://custom/template]) - end - end - context 'and provided a description for the task' do let(:generator_opts) do { diff --git a/spec/unit/pdk/generate/module_spec.rb b/spec/unit/pdk/generate/module_spec.rb index b38ccb29f..1583c9009 100644 --- a/spec/unit/pdk/generate/module_spec.rb +++ b/spec/unit/pdk/generate/module_spec.rb @@ -34,8 +34,10 @@ dir_double = instance_double(Pathname, mkpath: true, to_path: '/a/path') allow(dir_double).to receive(:+).with(anything).and_return(dir_double) allow(dir_double).to receive(:dirname).and_return(dir_double) - allow(Pathname).to receive(:new).with(temp_target_dir).and_return(dir_double) + allow(dir_double).to receive(:relative?).and_return(true) + allow(Pathname).to receive(:new).with(anything).and_return(dir_double) allow(File).to receive(:open).with(dir_double, 'wb').and_yield(test_template_file) + allow(PDK::Util::Git).to receive(:repo?).with(anything).and_return(true) end end @@ -178,11 +180,11 @@ context 'when a template-url is supplied on the command line' do before(:each) do allow(FileUtils).to receive(:mv).with(temp_target_dir, target_dir).and_return(0) - allow(PDK::Util).to receive(:default_template_url).and_return('https://github.com/puppetlabs/pdk-templates') + allow(PDK::Util).to receive(:default_template_uri).and_return(Addressable::URI.parse('https://github.com/puppetlabs/pdk-templates')) end it 'uses that template to generate the module' do - expect(PDK::Module::TemplateDir).to receive(:new).with('cli-template', anything, anything).and_yield(test_template_dir) + expect(PDK::Module::TemplateDir).to receive(:new).with(Addressable::URI.parse('cli-template'), anything, anything).and_yield(test_template_dir) expect(logger).to receive(:info).with(a_string_matching(%r{generated at path}i)) expect(logger).to receive(:info).with(a_string_matching(%r{In your module directory, add classes with the 'pdk new class' command}i)) @@ -191,22 +193,22 @@ it 'takes precedence over the template-url answer' do PDK.answers.update!('template-url' => 'answer-template') - expect(PDK::Module::TemplateDir).to receive(:new).with('cli-template', anything, anything).and_yield(test_template_dir) + expect(PDK::Module::TemplateDir).to receive(:new).with(Addressable::URI.parse('cli-template'), anything, anything).and_yield(test_template_dir) described_class.invoke(invoke_opts.merge(:'template-url' => 'cli-template')) end - it 'saves the template-url to the answer file if it is not the puppetlabs template' do - expect(PDK.answers).to receive(:update!).with('template-url' => 'cli-template') + it 'saves the template-url and template-ref to the answer file if it is not the default template' do + expect(PDK::Util).to receive(:template_uri).and_return(Addressable::URI.parse('cli-template')) + expect(PDK.answers).to receive(:update!).with('template-url' => Addressable::URI.parse('cli-template')) described_class.invoke(invoke_opts.merge(:'template-url' => 'cli-template')) end - it 'clears the saved template-url answer if it is the puppetlabs template' do - PDK.answers.update!('template-url' => 'custom-url') + it 'clears the saved template-url answer if it is the default template' do + expect(PDK::Util).to receive(:template_uri).and_return(Addressable::URI.parse('https://github.com/puppetlabs/pdk-templates')) expect(PDK.answers).to receive(:update!).with('template-url' => nil).and_call_original - allow(PDK::Util).to receive(:puppetlabs_template_url).and_return('puppetlabs-url') - described_class.invoke(invoke_opts.merge(:'template-url' => 'puppetlabs-url')) + described_class.invoke(invoke_opts.merge(:'template-url' => 'https://github.com/puppetlabs/pdk-templates')) expect(PDK.answers['template-url']).to eq(nil) end end @@ -214,16 +216,13 @@ context 'when a template-url is not supplied on the command line' do before(:each) do allow(FileUtils).to receive(:mv).with(temp_target_dir, target_dir).and_return(0) + allow(PDK::Util).to receive(:development_mode?).and_return(true) end context 'and a template-url answer exists' do - before(:each) do - allow(PDK::Util::Git).to receive(:repo?).with('answer-template').and_return(true) - end - it 'uses the template-url from the answer file to generate the module' do PDK.answers.update!('template-url' => 'answer-template') - expect(PDK::Module::TemplateDir).to receive(:new).with('answer-template', anything, anything).and_yield(test_template_dir) + expect(PDK::Module::TemplateDir).to receive(:new).with(Addressable::URI.parse('answer-template'), anything, anything).and_yield(test_template_dir) expect(logger).to receive(:info).with(a_string_matching(%r{generated at path}i)) expect(logger).to receive(:info).with(a_string_matching(%r{In your module directory, add classes with the 'pdk new class' command}i)) @@ -239,7 +238,7 @@ end it 'uses the vendored template url' do - expect(PDK::Module::TemplateDir).to receive(:new).with('file:///tmp/package/cache/pdk-templates.git', anything, anything).and_yield(test_template_dir) + expect(PDK::Module::TemplateDir).to receive(:new).with(Addressable::URI.parse('file:///tmp/package/cache/pdk-templates.git'), anything, anything).and_yield(test_template_dir) expect(PDK.answers).not_to receive(:update!).with(:'template-url' => anything) described_class.invoke(invoke_opts) @@ -248,7 +247,7 @@ context 'and pdk is not installed from packages' do it 'uses the default template to generate the module' do - expect(PDK::Module::TemplateDir).to receive(:new).with(PDK::Util.default_template_url, anything, anything).and_yield(test_template_dir) + expect(PDK::Module::TemplateDir).to receive(:new).with(PDK::Util.default_template_uri, anything, anything).and_yield(test_template_dir) expect(PDK.answers).not_to receive(:update!).with(:'template-url' => anything) described_class.invoke(invoke_opts) diff --git a/spec/unit/pdk/generate/puppet_object_spec.rb b/spec/unit/pdk/generate/puppet_object_spec.rb index 217fadb18..87bbf6c31 100644 --- a/spec/unit/pdk/generate/puppet_object_spec.rb +++ b/spec/unit/pdk/generate/puppet_object_spec.rb @@ -4,7 +4,6 @@ let(:module_metadata) { '{"name": "testuser-test_module"}' } before(:each) do - allow(PDK::Util).to receive(:package_install?).and_return(false) allow(File).to receive(:file?).with(metadata_path).and_return(true) allow(File).to receive(:readable?).with(metadata_path).and_return(true) allow(File).to receive(:read).with(metadata_path).and_return(module_metadata) @@ -21,6 +20,9 @@ before(:each) do stub_const('PDK::Generate::PuppetObject::OBJECT_TYPE', object_type) + allow(PDK::Util).to receive(:package_install?).and_return(false) + allow(PDK::Util).to receive(:module_root).and_return(module_dir) + allow(PDK).to receive(:answers).and_return({}) end describe '#template_data' do @@ -140,14 +142,14 @@ allow(default_templatedir).to receive(:object_config).and_return(configs_hash) allow(cli_templatedir).to receive(:object_config).and_return(configs_hash) allow(metadata_templatedir).to receive(:object_config).and_return(configs_hash) - allow(PDK::Module::TemplateDir).to receive(:new).with(PDK::Util.default_template_url).and_yield(default_templatedir) + allow(PDK::Module::TemplateDir).to receive(:new).with(PDK::Util.default_template_uri).and_yield(default_templatedir) end context 'when a template-url is provided on the CLI' do let(:options) { { :'template-url' => '/some/path' } } before(:each) do - allow(PDK::Module::TemplateDir).to receive(:new).with(options[:'template-url']).and_yield(cli_templatedir) + allow(PDK::Module::TemplateDir).to receive(:new).with(Addressable::URI.parse('/some/path')).and_yield(cli_templatedir) end context 'and a template for the object type exists' do @@ -176,7 +178,7 @@ let(:module_metadata) { '{"name": "testuser-test_module", "template-url": "/some/other/path"}' } before(:each) do - allow(PDK::Module::TemplateDir).to receive(:new).with('/some/other/path').and_yield(metadata_templatedir) + allow(PDK::Module::TemplateDir).to receive(:new).with(Addressable::URI.parse('/some/other/path')).and_yield(metadata_templatedir) end context 'and a template for the object type exists' do diff --git a/spec/unit/pdk/module/convert_spec.rb b/spec/unit/pdk/module/convert_spec.rb index 0350d28d4..840017c49 100644 --- a/spec/unit/pdk/module/convert_spec.rb +++ b/spec/unit/pdk/module/convert_spec.rb @@ -91,6 +91,7 @@ allow(PDK::Module::UpdateManager).to receive(:new).and_return(update_manager) allow(instance).to receive(:update_metadata).with(any_args).and_return(metadata) allow(PDK::Module::TemplateDir).to receive(:new).with(anything, anything, anything).and_yield(template_dir) + allow(PDK::Util::Git).to receive(:repo?).with(anything).and_return(true) allow(template_dir).to receive(:module_metadata=) allow(template_dir).to receive(:render).and_yield(template_files[:path], template_files[:content], template_files[:status]) allow(update_manager).to receive(:changes).and_return(changes) diff --git a/spec/unit/pdk/module/template_dir_spec.rb b/spec/unit/pdk/module/template_dir_spec.rb index e845829ea..5284a858d 100644 --- a/spec/unit/pdk/module/template_dir_spec.rb +++ b/spec/unit/pdk/module/template_dir_spec.rb @@ -3,13 +3,14 @@ describe PDK::Module::TemplateDir do subject(:template_dir) do - described_class.new(path_or_url, module_metadata, true) do |foo| + described_class.new(uri, module_metadata, true) do |foo| # block does nothing end end let(:root_dir) { Gem.win_platform? ? 'C:/' : '/' } let(:path_or_url) { File.join(root_dir, 'path', 'to', 'templates') } + let(:uri) { Gem.win_platform? ? Addressable::URI.parse('/' + path_or_url) : Addressable::URI.parse(path_or_url) } let(:tmp_path) { File.join(root_dir, 'tmp', 'path') } let(:module_metadata) do @@ -40,7 +41,7 @@ allow(File).to receive(:read).with(File.join(path_or_url, 'config_defaults.yml')).and_return(config_defaults) allow(Dir).to receive(:rmdir).with(tmp_path).and_return(0) - allow(described_class).to receive(:new).with(path_or_url, module_metadata).and_yield(template_dir) + allow(described_class).to receive(:new).with(uri, module_metadata).and_yield(template_dir) expect(template_dir.object_config).to include('module_metadata' => module_metadata) end end @@ -329,9 +330,13 @@ allow(PDK::Util).to receive(:make_tmpdir_name).with('pdk-templates').and_return(tmp_path) allow(Dir).to receive(:chdir).with(tmp_path).and_yield allow(PDK::Util::Git).to receive(:git).with('clone', path_or_url, tmp_path).and_return(exit_code: 0) - allow(PDK::Util::Git).to receive(:git).with('reset', '--hard', 'default-ref').and_return(exit_code: 0) + allow(PDK::Util::Git).to receive(:git).with('reset', '--hard', 'default-sha').and_return(exit_code: 0) allow(FileUtils).to receive(:remove_dir).with(tmp_path) allow(PDK::Util::Git).to receive(:git).with('--git-dir', anything, 'describe', '--all', '--long', '--always').and_return(exit_code: 0, stdout: '1234abcd') + allow(PDK::Util::Git).to receive(:git).with('--work-tree', anything, '--git-dir', anything, 'status', '--untracked-files=no', '--porcelain', anything).and_return(exit_code: 0, stdout: '') + allow(PDK::Util::Git).to receive(:git).with('--git-dir', anything, 'ls-remote', '--refs', 'origin', 'default-ref').and_return(exit_code: 0, stdout: + "default-sha\trefs/heads/default-ref\n" \ + "default-sha\trefs/remotes/origin/default-ref") allow(PDK::Util::Version).to receive(:version_string).and_return('0.0.0') allow(PDK::Util).to receive(:canonical_path).with(tmp_path).and_return(tmp_path) end @@ -352,9 +357,13 @@ allow(PDK::Util).to receive(:make_tmpdir_name).with('pdk-templates').and_return(tmp_path) allow(Dir).to receive(:chdir).with(tmp_path).and_yield allow(PDK::Util::Git).to receive(:git).with('clone', path_or_url, tmp_path).and_return(exit_code: 0) - allow(PDK::Util::Git).to receive(:git).with('reset', '--hard', 'origin/master').and_return(exit_code: 0) + allow(PDK::Util::Git).to receive(:git).with('reset', '--hard', 'default-sha').and_return(exit_code: 0) allow(FileUtils).to receive(:remove_dir).with(tmp_path) allow(PDK::Util::Git).to receive(:git).with('--git-dir', anything, 'describe', '--all', '--long', '--always').and_return(exit_code: 0, stdout: '1234abcd') + allow(PDK::Util::Git).to receive(:git).with('--work-tree', anything, '--git-dir', anything, 'status', '--untracked-files=no', '--porcelain', anything).and_return(exit_code: 0, stdout: '') + allow(PDK::Util::Git).to receive(:git).with('--git-dir', anything, 'ls-remote', '--refs', 'origin', 'default-ref').and_return(exit_code: 0, stdout: + "default-sha\trefs/heads/default-ref\n" \ + "default-sha\trefs/remotes/origin/default-ref") allow(PDK::Util::Version).to receive(:version_string).and_return('0.0.0') allow(PDK::Util).to receive(:canonical_path).with(tmp_path).and_return(tmp_path) end diff --git a/spec/unit/pdk/module/update_spec.rb b/spec/unit/pdk/module/update_spec.rb index b87d39c57..cc85dc665 100644 --- a/spec/unit/pdk/module/update_spec.rb +++ b/spec/unit/pdk/module/update_spec.rb @@ -7,6 +7,7 @@ instance_double( PDK::Module::Metadata, data: { + 'name' => 'mock-module', 'template-url' => template_url, 'template-ref' => template_ref, }, @@ -98,7 +99,7 @@ context 'when using the default template' do let(:options) { { noop: true } } - let(:template_url) { PDK::Util.default_template_url } + let(:template_url) { PDK::Util.default_template_uri } it 'refers to the template as the default template' do expect(logger).to receive(:info).with(a_string_matching(%r{using the default template}i)) @@ -201,8 +202,8 @@ end end - describe '#template_url' do - subject { described_class.new(options).template_url } + describe '#metadata_template_uri' do + subject { described_class.new(options).metadata_template_uri.to_s } include_context 'with mock metadata' @@ -240,24 +241,24 @@ context 'when the default_template_ref specifies a tag' do before(:each) do - allow(PDK::Util).to receive(:default_template_ref).and_return('1.4.0') + allow(PDK::Util).to receive(:default_template_ref).and_return(PDK::TEMPLATE_REF) end it 'returns the tag name' do - is_expected.to eq('1.4.0') + is_expected.to eq(PDK::TEMPLATE_REF) end end context 'when the default_template_ref specifies a branch head' do before(:each) do - allow(PDK::Util).to receive(:default_template_ref).and_return('origin/master') + allow(PDK::Util).to receive(:default_template_ref).and_return('master') allow(PDK::Util::Git).to receive(:ls_remote) - .with(template_url, 'refs/heads/master') + .with(template_url, 'master') .and_return('3cdd84e8f0aae30bf40d15556482fc8752899312') end include_context 'with mock metadata' - let(:template_ref) { 'heads/master-0-g07678c8' } + let(:template_ref) { 'master-0-g07678c8' } it 'returns the branch name and the commit SHA' do is_expected.to eq('master@3cdd84e') diff --git a/spec/unit/pdk/util/git_spec.rb b/spec/unit/pdk/util/git_spec.rb index f641eb8ac..d7f4ad094 100644 --- a/spec/unit/pdk/util/git_spec.rb +++ b/spec/unit/pdk/util/git_spec.rb @@ -65,7 +65,7 @@ subject { described_class.ls_remote(repo, ref) } let(:repo) { 'https://github.com/puppetlabs/pdk-templates' } - let(:ref) { 'refs/heads/master' } + let(:ref) { 'master' } before(:each) do allow(described_class).to receive(:git).with('ls-remote', '--refs', repo, ref).and_return(git_result) diff --git a/spec/unit/pdk/util_spec.rb b/spec/unit/pdk/util_spec.rb index 3f7b96e27..c177f7299 100644 --- a/spec/unit/pdk/util_spec.rb +++ b/spec/unit/pdk/util_spec.rb @@ -2,7 +2,7 @@ describe PDK::Util do let(:pdk_version) { '1.2.3' } - let(:template_url) { 'https://github.com/puppetlabs/pdk-templates' } + let(:template_url) { 'metadata-templates' } let(:template_ref) { nil } let(:mock_metadata) do instance_double( @@ -363,118 +363,310 @@ end end - describe '.default_template_url' do - subject { described_class.default_template_url } + describe '.template_url' do + let(:uri) { Addressable::URI.parse(template_url) } - before(:each) do - allow(described_class).to receive(:puppetlabs_template_url).and_return('puppetlabs_template_url') + context 'when the uri has a fragment' do + let(:template_url) { 'https://github.com/my/pdk-templates.git#custom' } + + it 'returns just the url portion' do + expect(described_class.template_url(uri)).to eq 'https://github.com/my/pdk-templates.git' + end end - context 'when there is no template-url in answers file' do - before(:each) do - allow(PDK).to receive(:answers).and_return('template-url' => nil) + context 'when the uri has no fragment' do + let(:template_url) { 'https://github.com/my/pdk-templates.git' } + + it 'returns just the url portion' do + expect(described_class.template_url(uri)).to eq 'https://github.com/my/pdk-templates.git' end + end - it 'returns puppetlabs template url' do - is_expected.to eq('puppetlabs_template_url') + context 'when the uri is an absolute path' do + context 'on linux' do + let(:template_url) { '/my/pdk-templates.git#custom' } + + it 'returns url portion' do + allow(Gem).to receive(:win_platform?).and_return(false) + expect(described_class.template_url(uri)).to eq '/my/pdk-templates.git' + end + end + context 'on windows' do + let(:template_url) { '/C:/my/pdk-templates.git#custom' } + + it 'returns url portion' do + allow(Gem).to receive(:win_platform?).and_return(true) + expect(described_class.template_url(uri)).to eq 'C:/my/pdk-templates.git' + end end end + end - context 'when the template-url in answers file matches current puppetlabs template' do - before(:each) do - allow(PDK).to receive(:answers).and_return('template-url' => 'puppetlabs_template_url') + describe '.template_ref' do + let(:uri) { Addressable::URI.parse(template_url) } + + context 'when the uri has a fragment' do + let(:template_url) { 'https://github.com/my/pdk-templates.git#custom' } + + it 'returns just the ref portion' do + expect(described_class.template_ref(uri)).to eq 'custom' end + end - it 'returns puppetlabs template url' do - is_expected.to eq('puppetlabs_template_url') + context 'when the uri has no fragment' do + let(:template_url) { 'https://github.com/my/pdk-templates.git' } + + it 'returns the default ref' do + expect(described_class.template_ref(uri)).to eq described_class.default_template_ref end end + end - context 'when the template-url in answers file matches old puppetlabs template' do - before(:each) do - allow(PDK).to receive(:answers).and_return('template-url' => 'https://github.com/puppetlabs/pdk-module-template') + describe '.template_path' do + let(:uri) { Addressable::URI.parse(template_url) } + + context 'when the uri has a schema' do + context 'on linux' do + let(:template_url) { 'file:///my/pdk-templates.git#fragment' } + + it 'returns the path' do + allow(Gem).to receive(:win_platform?).and_return(false) + expect(described_class.template_path(uri)).to eq '/my/pdk-templates.git' + end end - it 'returns puppetlabs template url' do - is_expected.to eq('puppetlabs_template_url') + context 'on windows' do + let(:template_url) { 'file:///C:/my/pdk-templates.git#fragment' } + + it 'returns the path' do + allow(Gem).to receive(:win_platform?).and_return(true) + expect(described_class.template_path(uri)).to eq 'C:/my/pdk-templates.git' + end end end - context 'when the template-url in answers file is custom' do - before(:each) do - allow(PDK).to receive(:answers).and_return('template-url' => 'custom_template_url') + context 'when the uri is just an absolute path' do + context 'on linux' do + let(:template_url) { '/my/pdk-templates.git#custom' } + + it 'returns url portion' do + allow(Gem).to receive(:win_platform?).and_return(false) + expect(described_class.template_url(uri)).to eq '/my/pdk-templates.git' + end + end + context 'on windows' do + let(:template_url) { '/C:/my/pdk-templates.git#custom' } + + it 'returns url portion' do + allow(Gem).to receive(:win_platform?).and_return(true) + expect(described_class.template_url(uri)).to eq 'C:/my/pdk-templates.git' + end end + end + end - context 'and the template is a directory' do + describe '.template_uri' do + before(:each) do + allow(PDK::Util::Git).to receive(:repo?).with(anything).and_return(true) + allow(described_class).to receive(:module_root).and_return('/path/to/module') + end + + context 'when passed no options' do + context 'and there are no metadata or answers' do before(:each) do - allow(File).to receive(:directory?).with('custom_template_url').and_return(true) - allow(described_class).to receive(:package_install?).and_return(false) + PDK.answers.update!('template-url' => nil) end - let(:moduleroot_dir) { File.join('custom_template_url', 'moduleroot') } - let(:moduleroot_init) { File.join('custom_template_url', 'moduleroot_init') } + it 'returns the default template' do + expect(described_class.template_uri({})).to eq(described_class.default_template_uri) + end + end - context 'that contains a valid PDK template' do - before(:each) do - allow(File).to receive(:directory?).with(moduleroot_dir).and_return(true) - allow(File).to receive(:directory?).with(moduleroot_init).and_return(true) - end + context 'and there are only answers' do + before(:each) do + PDK.answers.update!('template-url' => 'answer-templates') + end - it 'returns the custom url' do - is_expected.to eq('custom_template_url') - end + it 'returns the answers template' do + expect(described_class.template_uri({})).to eq(Addressable::URI.parse('answer-templates')) end - context 'that does not contain a valid PDK template' do + context 'and the answer file template is invalid' do before(:each) do - allow(File).to receive(:directory?).with(moduleroot_dir).and_return(false) + allow(described_class).to receive(:valid_template?).with(anything).and_call_original + allow(described_class).to receive(:valid_template?).with(uri: anything, allow_fallback: true, type: anything).and_return(false) end - it 'returns the puppetlabs template url' do - expect(PDK.answers).to receive(:update!).with('template-url' => nil) - is_expected.to eq('puppetlabs_template_url') + it 'returns the default template' do + expect(described_class.template_uri({})).to eq(described_class.default_template_uri) end end end - context 'and the template is a valid repo' do + context 'and there are metadata and answers' do before(:each) do - allow(PDK::Util::Git).to receive(:repo?).with('custom_template_url').and_return(true) + PDK.answers.update!('template-url' => 'answer-templates') + end + + it 'returns the metadata template' do + allow(PDK::Module::Metadata).to receive(:from_file).with('/path/to/module/metadata.json').and_return(mock_metadata) + allow(File).to receive(:file?).with('/path/to/module/metadata.json').and_return(true) + allow(File).to receive(:file?).with(%r{PDK_VERSION}).and_return(true) + expect(described_class.template_uri({})).to eq(Addressable::URI.parse('metadata-templates')) + end + end + end + + context 'when there are metadata and answers' do + before(:each) do + PDK.answers.update!('template-url' => 'answer-templates') + allow(PDK::Module::Metadata).to receive(:from_file).with(File.join(described_class.module_root, 'metadata.json')).and_return(mock_metadata) + end + + context 'and passed template-url' do + it 'returns the specified template' do + expect(described_class.template_uri(:'template-url' => 'cli-templates')).to eq(Addressable::URI.parse('cli-templates')) + end + end + + context 'and passed windows template-url' do + it 'returns the specified template' do + allow(Gem).to receive(:win_platform?).and_return(true) + expect(described_class.template_uri(:'template-url' => 'C:\cli-templates')).to eq(Addressable::URI.parse('/C:\cli-templates')) end + end + + context 'and passed template-ref' do + it 'errors because it requires url with ref' do + expect { described_class.template_uri(:'template-ref' => 'cli-ref') }.to raise_error(PDK::CLI::FatalError, %r{--template-ref requires --template-url}) + end + end - it 'returns custom url' do - is_expected.to eq('custom_template_url') + context 'and passed template-url and template-ref' do + it 'returns the specified template and ref' do + uri = Addressable::URI.parse('cli-templates') + uri.fragment = 'cli-ref' + expect(described_class.template_uri(:'template-url' => 'cli-templates', :'template-ref' => 'cli-ref')).to eq(uri) end end + end + end + + describe '.valid_template?' do + subject { described_class.valid_template?(template) } + + context 'when template is nil' do + let(:template) { nil } + + it { is_expected.to be_falsey } + end + + context 'when template is a Hash' do + context 'and template[:uri] => nil' do + let(:template) { { uri: nil } } + + it { is_expected.to be_falsey } + end + + context 'and template[:path] => directory' do + let(:template) do + { + uri: Addressable::URI.parse('file:///a/template/on/disk'), + path: '/a/template/on/disk', + } + end - context 'and the template is not a valid repo' do before(:each) do - allow(PDK::Util::Git).to receive(:repo?).with('custom_template_url').and_return(false) - allow(PDK.answers).to receive(:update!).with('template-url' => nil) + allow(Gem).to receive(:win_platform?).and_return(true) + allow(described_class).to receive(:package_install?).and_return(false) + allow(PDK::Util::Git).to receive(:repo?).with(template[:uri]).and_return(false) + end + + context 'that exists and contains a valid template' do + before(:each) do + allow(File).to receive(:directory?).with(template[:path]).and_return(true) + allow(File).to receive(:directory?).with(File.join(template[:path], 'moduleroot')).and_return(true) + allow(File).to receive(:directory?).with(File.join(template[:path], 'moduleroot_init')).and_return(true) + end + + it { is_expected.to be_truthy } + end + + context 'that exists and does not contain a valid template' do + before(:each) do + allow(File).to receive(:directory?).with(template[:path]).and_return(true) + allow(File).to receive(:directory?).with(File.join(template[:path], 'moduleroot')).and_return(false) + allow(File).to receive(:directory?).with(File.join(template[:path], 'moduleroot_init')).and_return(false) + end + + it { is_expected.to be_falsey } + end + + context 'that does not exist' do + before(:each) do + allow(File).to receive(:directory?).with(template[:path]).and_return(false) + end + + it { is_expected.to be_falsey } end + end + + context 'and template[:uri] => URI' do + let(:template) { { uri: Addressable::URI.parse('https://this.is/a/repo.git') } } - it 'returns the puppetlabs template url' do - expect(logger).to receive(:warn).with(a_string_matching(%r{using the default template})) - is_expected.to eq('puppetlabs_template_url') + context 'that points to a git repo' do + before(:each) do + allow(PDK::Util::Git).to receive(:repo?).with(template[:uri]).and_return(true) + end + + it { is_expected.to be_truthy } + end + + context 'that does not point to a git repo' do + before(:each) do + allow(PDK::Util::Git).to receive(:repo?).with(template[:uri]).and_return(false) + end + + it { is_expected.to be_falsey } end end end end - describe '.default_template_ref' do - subject { described_class.default_template_ref } + describe '.default_template_uri' do + subject { described_class.default_template_uri } - before(:each) do - allow(described_class).to receive(:puppetlabs_template_url).and_return('puppetlabs_template_url') + context 'when it is a package install' do + before(:each) do + allow(described_class).to receive(:package_install?).and_return(true) + end + + it 'returns the file template repo' do + allow(described_class).to receive(:package_cachedir).and_return('/path/to/pdk') + is_expected.to eq(Addressable::URI.parse('file:///path/to/pdk/pdk-templates.git')) + end + end + context 'when it is not a package install' do + before(:each) do + allow(described_class).to receive(:package_install?).and_return(false) + end + + it 'returns puppetlabs template url' do + is_expected.to eq(Addressable::URI.parse('https://github.com/puppetlabs/pdk-templates')) + end end + end + + describe '.default_template_ref' do + subject { described_class.default_template_ref } context 'with a custom template repo' do before(:each) do allow(described_class).to receive(:default_template_url).and_return('custom_template_url') end - it 'returns origin/master' do - is_expected.to eq('origin/master') + it 'returns master' do + is_expected.to eq('master') end end @@ -498,8 +690,8 @@ allow(described_class).to receive(:development_mode?).and_return(true) end - it 'returns origin/master' do - is_expected.to eq('origin/master') + it 'returns master' do + is_expected.to eq('master') end end end From 1b770010793cf6cc9db88886650726103733f2cc Mon Sep 17 00:00:00 2001 From: Hunter Haugen Date: Fri, 8 Jun 2018 14:58:47 -0700 Subject: [PATCH 02/24] (WIP) today's work rewriting template uri handling methods --- lib/pdk/generate/module.rb | 6 +- lib/pdk/module/convert.rb | 2 +- lib/pdk/module/templatedir.rb | 11 +- lib/pdk/module/update.rb | 2 +- lib/pdk/util.rb | 160 +------------------------ lib/pdk/util/git.rb | 10 +- lib/pdk/util/template_uri.rb | 214 ++++++++++++++++++++++++++++++++++ 7 files changed, 226 insertions(+), 179 deletions(-) create mode 100644 lib/pdk/util/template_uri.rb diff --git a/lib/pdk/generate/module.rb b/lib/pdk/generate/module.rb index bcf62c7d7..3cb4126ae 100644 --- a/lib/pdk/generate/module.rb +++ b/lib/pdk/generate/module.rb @@ -54,7 +54,7 @@ def self.invoke(opts = {}) prepare_module_directory(temp_target_dir) - template_uri = PDK::Util.template_uri(opts) + template_uri = PDK::Util::TemplateURI.new(opts) begin PDK::Module::TemplateDir.new(template_uri, metadata.data, true) do |templates| @@ -81,8 +81,8 @@ def self.invoke(opts = {}) # resolution can find new default URLs in the future. PDK.answers.update!('template-url' => nil) if opts.key?(:'template-url') else - # Save the template-url + template-ref answers if the module was - # generated using a template/reference other than ours. + # Save the template-url answers if the module was generated using a + # template/reference other than ours. PDK.answers.update!('template-url' => template_uri) end diff --git a/lib/pdk/module/convert.rb b/lib/pdk/module/convert.rb index 6db33d635..40ab51030 100644 --- a/lib/pdk/module/convert.rb +++ b/lib/pdk/module/convert.rb @@ -103,7 +103,7 @@ def update_manager end def template_uri - @template_uri ||= PDK::Util.template_uri(options) + @template_uri ||= PDK::Util::TemplateURI.new(options) end def update_metadata(metadata_path, template_metadata) diff --git a/lib/pdk/module/templatedir.rb b/lib/pdk/module/templatedir.rb index 23f1001e3..460d92202 100644 --- a/lib/pdk/module/templatedir.rb +++ b/lib/pdk/module/templatedir.rb @@ -16,7 +16,7 @@ class TemplateDir # The template directory is only guaranteed to be available on disk # within the scope of the block passed to this method. # - # @param uri [Addressable::URI] The path to a directory to use as the + # @param uri [PDK::Util::TemplateURI] The path to a directory to use as the # template or a URI to a git repository. # @param module_metadata [Hash] A Hash containing the module metadata. # Defaults to an empty Hash. @@ -41,21 +41,20 @@ def initialize(uri, module_metadata = {}, init = false) unless block_given? raise ArgumentError, _('%{class_name} must be initialized with a block.') % { class_name: self.class.name } end - unless uri.is_a? Addressable::URI - raise ArgumentError, _('PDK::Module::TemplateDir.new must be initialized with an Addressable::URI, got a %{uri_type}') % { uri_type: uri.class } + unless uri.is_a? PDK::Util::TemplateURI + raise ArgumentError, _('PDK::Module::TemplateDir.new must be initialized with a PDK::Util::TemplateURI, got a %{uri_type}') % { uri_type: uri.class } end - if PDK::Util::Git.repo?(PDK::Util.template_url(uri)) + if PDK::Util::Git.repo?(uri.location) # This is either a bare local repo or a remote. either way it needs cloning. @path = self.class.clone_template_repo(uri) temp_dir_clone = true else # if it is a local path & non-bare repo then we can use it directly. # Still have to check the branch. - @path = PDK::Util.template_path(uri) + @path = uri.shell_path # We don't do a checkout of local-path repos. There are lots of edge # cases or user un-expectations. - # self.class.checkout_template_ref(@path, PDK::Util.template_ref(uri)) PDK.logger.warn _("Repository '%{repo}' has a work-tree; skipping git reset.") % { repo: @path } end @cloned_from = PDK::Util.deuri_path(uri.to_s) diff --git a/lib/pdk/module/update.rb b/lib/pdk/module/update.rb index 746fddec6..82334f73c 100644 --- a/lib/pdk/module/update.rb +++ b/lib/pdk/module/update.rb @@ -54,7 +54,7 @@ def module_metadata # tracked if existing, but the template-ref in the metadata is not for # tracking simply for referencing the last update target. def metadata_template_uri - @metadata_template_uri ||= Addressable::URI.parse(module_metadata.data['template-url']) + @metadata_template_uri ||= PDK::Util::TemplateURI.new(module_metadata.data['template-url']) end def current_version diff --git a/lib/pdk/util.rb b/lib/pdk/util.rb index bf5ae23db..7b7956de2 100644 --- a/lib/pdk/util.rb +++ b/lib/pdk/util.rb @@ -5,6 +5,7 @@ require 'pdk/util/windows' require 'pdk/util/vendored_file' require 'pdk/util/filesystem' +require 'pdk/util/template_uri' module PDK module Util @@ -197,165 +198,6 @@ def targets_relative_to_pwd(targets) end module_function :targets_relative_to_pwd - # @returns String - def template_url(uri) - if uri.is_a?(Addressable::URI) && uri.fragment - deuri_path(uri.to_s.chomp('#' + uri.fragment)) - else - deuri_path(uri.to_s) - end - end - module_function :template_url - # @returns String - def template_ref(uri) - if uri.fragment - uri.fragment - else - default_template_ref - end - end - module_function :template_ref - # @returns String - def template_path(uri) - deuri_path(uri.path) - end - module_function :template_path - # C:\... urls are not URI-safe. They should be /C:\... to be so. - # @returns String - def uri_path(path) - (Gem.win_platform? && path =~ %r{^[a-zA-Z][\|:]}) ? "/#{path}" : path - end - module_function :uri_path - # @returns String - def deuri_path(path) - (Gem.win_platform? && path =~ %r{^\/[a-zA-Z][\|:]}) ? path[1..-1] : path - end - module_function :deuri_path - - # @returns Addressable::URI - def template_uri(opts) - # 1. Get the four sources of URIs - # 2. Pick the first non-nil URI - # 3. Error if the URI is not a valid git repo (missing directory or http 404) - # 4. Leave updating answers/metadata to other code - # XXX Make it work before making it pretty. - found_template = templates(opts).find { |t| valid_template?(t) } - - raise PDK::CLI::FatalError, _('Unable to find a valid module template to use.') if found_template.nil? - found_template[:uri] - end - module_function :template_uri - - def valid_template?(template) - return false if template.nil? - return false if template[:uri].nil? - - if template[:path] && File.directory?(template[:path]) - PDK::Module::TemplateDir.new(template[:uri]) {} - return true - end - return true if PDK::Util::Git.repo?(template[:uri]) - - false - rescue ArgumentError - return false - end - module_function :valid_template? - - # @return [Array Object}>] an array of hashes. Each hash - # contains 3 keys: :type contains a String that describes the template - # directory, :url contains a String with the URL to the template - # directory, and :allow_fallback contains a Boolean that specifies if - # the lookup process should proceed to the next template directory if - # the template file is not in this template directory. - # - def templates(opts) - explicit_url = opts.fetch(:'template-url', nil) - explicit_ref = opts.fetch(:'template-ref', nil) - - if explicit_ref && explicit_url.nil? - raise PDK::CLI::FatalError, _('--template-ref requires --template-url to also be specified.') - end - if explicit_url && explicit_url.include?('#') - raise PDK::CLI::FatalError, _('--template-url may not be used to specify paths containing #\'s') - end - - # 1. Get the CLI, metadata (or answers if no metadata), and default URIs - # 2. Construct the hash - if explicit_url - # Valid URIs to avoid catching: - # - absolute local paths - # - have :'s in paths when preceeded by a slash - # - have only digits following the : and preceeding a / or end-of-string that is 0-65535 - # The last item is ambiguous in the case of scp/git paths vs. URI port - # numbers, but can be made unambiguous by making the form to - # ssh://git@github.com/1234/repo.git or - # ssh://git@github.com:1234/user/repo.git - scp_url_m = explicit_url.match(%r{\A(.*@[^/:]+):(.+)\z}) - if Pathname.new(explicit_url).relative? && scp_url_m - # "^git@..." is also malformed as it is missing a scheme - # "ssh://git@..." is correct. - check_url = Addressable::URI.parse(scp_url_m[1]) - scheme = 'ssh://' unless check_url.scheme - - numbers_m = scp_url_m[2].split('/')[0].match(%r{\A[0-9]+\z}) - if numbers_m && numbers_m[0].to_i < 65_536 - # consider it an explicit-port URI, even though it's ambiguous. - explicit_url = Addressable::URI.parse(scheme + scp_url_m[1] + ':' + scp_url_m[2]) - else - explicit_url = Addressable::URI.parse(scheme + scp_url_m[1] + '/' + scp_url_m[2]) - PDK.logger.warn(_('--template-url appears to be an SCP-style url; it will be converted to an RFC-compliant URI: %{uri}') % { uri: explicit_url }) - end - end - explicit_uri = Addressable::URI.parse(uri_path(explicit_url)) - explicit_uri.fragment = explicit_ref - else - explicit_uri = nil - end - metadata_uri = if module_root && File.file?(File.join(module_root, 'metadata.json')) - Addressable::URI.parse(uri_path(module_metadata['template-url'])) - else - nil - end - answers_uri = if PDK.answers['template-url'] == 'https://github.com/puppetlabs/pdk-module-template' - # use the new github template-url if it is still the old one. - default_template_uri - elsif PDK.answers['template-url'] - Addressable::URI.parse(uri_path(PDK.answers['template-url'])) - else - nil - end - default_uri = default_template_uri - - ary = [] - ary << { type: _('--template-url'), uri: explicit_uri, allow_fallback: false } - ary << { type: _('metadata.json'), uri: metadata_uri, allow_fallback: true } if metadata_uri - ary << { type: _('PDK answers'), uri: answers_uri, allow_fallback: true } unless metadata_uri - ary << { type: _('default'), uri: default_uri, allow_fallback: false } - ary - end - module_function :templates - - # @returns Addressable::URI - def default_template_uri - if package_install? - Addressable::URI.new(scheme: 'file', host: '', path: File.join(package_cachedir, 'pdk-templates.git')) - else - Addressable::URI.parse('https://github.com/puppetlabs/pdk-templates') - end - end - module_function :default_template_uri - - # @returns String - def default_template_ref - if PDK::Util.development_mode? - 'master' - else - PDK::TEMPLATE_REF - end - end - module_function :default_template_ref - # TO-DO: Refactor replacement of lib/pdk/module/build.rb:metadata to use this function instead def module_metadata PDK::Module::Metadata.from_file(File.join(module_root, 'metadata.json')).data diff --git a/lib/pdk/util/git.rb b/lib/pdk/util/git.rb index 8a4115bc1..0d82d532e 100644 --- a/lib/pdk/util/git.rb +++ b/lib/pdk/util/git.rb @@ -39,8 +39,7 @@ def self.git_with_env(env, *args) end def self.repo?(maybe_repo) - # The behavior of file://... paths remote appears to be on purpose. - return bare_repo?(maybe_repo) if File.directory?(maybe_repo) # || (uri.scheme == 'file' && File.directory?(PDK::Util.template_path(uri))) + return bare_repo?(maybe_repo) if File.directory?(maybe_repo) remote_repo?(maybe_repo) end @@ -63,13 +62,6 @@ def self.work_dir_clean?(repo) git('--work-tree', repo, '--git-dir', File.join(repo, '.git'), 'status', '--untracked-files=no', '--porcelain', repo)[:stdout].empty? end - def self.current_branch(repo) - env = { 'GIT_DIR' => repo } - rev_parse = git_with_env(env, 'rev-parse', '--abbrev-ref', 'HEAD') - - rev_parse[:stdout].strip - end - def self.ls_remote(repo, ref) output = if File.directory?(repo) # Appveyor doesn't like windows-path repository targets diff --git a/lib/pdk/util/template_uri.rb b/lib/pdk/util/template_uri.rb new file mode 100644 index 000000000..ed0f8073f --- /dev/null +++ b/lib/pdk/util/template_uri.rb @@ -0,0 +1,214 @@ +require 'pdk/util' + +module PDK + module Util + class TemplateURI + # XXX Previously + # - template_uri used to get the string form of the uri when generating the module and written to pdk answers and metadata + # - template_path or deuri_path used for humans to see and commands to run + # - uri_path used only internally by the template selection code; move out + # - template_ref used by git checkout + attr_reader :uri + + # input/output formats: + # + # file:///c:/foo (git clone location) + # c:/foo (shell paths) + # file:///c:/foo#master (only for metadata) + # c:/foo#master (only for metadata) + # + # non output formats: + # + # /c:/foo (internal use only) + # /c:/foo#master (internal use only) + # + def initialize(opts_or_uri) + # If a uri string is passed, skip the valid uri finding code. + @uri = if opts_or_uri.is_a?(String) + begin + Addressable::URI.parse(opts_or_uri) + rescue Addressable::URI::InvalidURIError + raise PDK::CLI::FatalError, _('PDK::Util::TemplateURI attempted initialization with a non-uri string: {string}') % { string: opts_or_uri } + end + else + first_valid_uri(templates(opts_or_uri)) + end + end + + # This is the URI represented in a format suitable for writing to + # metadata. + # + # @returns String + def url + human_readable(@uri.to_s) + end + alias to_s url + + # This is the url without a fragment, suitable for git clones. I couldn't + # think of a better name. + # + # @returns String + def location + if @uri.is_a?(Addressable::URI) && @uri.fragment + human_readable(@uri.to_s.chomp('#' + @uri.fragment)) + else + human_readable(@uri.to_s) + end + end + + # This is the path of the URI, suitable for accessing directly from the shell. + # @returns String + def shell_path + human_readable(@uri.path) + end + + # @returns String + def git_ref + if @uri.fragment + @uri.fragment + else + default_template_ref + end + end + + private + + # `C:...` urls are not URI-safe. They should be of the form `/C:...` to + # be URI-safe. scp-like urls like `user@host:/path` are not URI-safe + # either but are not handled here. Should they be? + # + # @returns String + def uri_safe(string) + (Gem.win_platform? && string =~ %r{^[a-zA-Z][\|:]}) ? "/#{string}" : string + end + + # If the passed value is a URI-safe windows path such as `/C:...` then it + # should be changed to a human-friendly `C:...` form. Otherwise the + # passed value is left alone. + # + # @returns String + def human_readable(string) + (Gem.win_platform? && string =~ %r{^\/[a-zA-Z][\|:]}) ? string[1..-1] : string + end + + # @returns Addressable::URI + def first_valid_uri(templates_array) + # 1. Get the four sources of URIs + # 2. Pick the first non-nil URI + # 3. Error if the URI is not a valid git repo (missing directory or http 404) + # 4. Leave updating answers/metadata to other code + found_template = templates_array.find { |t| valid_template?(t) } + + raise PDK::CLI::FatalError, _('Unable to find a valid module template to use.') if found_template.nil? + found_template[:uri] + end + + def valid_template?(template) + return false if template.nil? + return false if template[:uri].nil? + + if template[:path] && File.directory?(template[:path]) + PDK::Module::TemplateDir.new(template[:uri]) {} + return true + end + return true if PDK::Util::Git.repo?(template[:uri]) + + false + rescue ArgumentError + return false + end + + # @return [Array Object}>] an array of hashes. Each hash + # contains 3 keys: :type contains a String that describes the template + # directory, :url contains a String with the URL to the template + # directory, and :allow_fallback contains a Boolean that specifies if + # the lookup process should proceed to the next template directory if + # the template file is not in this template directory. + # + def templates(opts) + explicit_url = opts.fetch(:'template-url', nil) + explicit_ref = opts.fetch(:'template-ref', nil) + + if explicit_ref && explicit_url.nil? + raise PDK::CLI::FatalError, _('--template-ref requires --template-url to also be specified.') + end + if explicit_url && explicit_url.include?('#') + raise PDK::CLI::FatalError, _('--template-url may not be used to specify paths containing #\'s') + end + + # 1. Get the CLI, metadata (or answers if no metadata), and default URIs + # 2. Construct the hash + if explicit_url + # Valid URIs to avoid catching: + # - absolute local paths + # - have :'s in paths when preceeded by a slash + # - have only digits following the : and preceeding a / or end-of-string that is 0-65535 + # The last item is ambiguous in the case of scp/git paths vs. URI port + # numbers, but can be made unambiguous by making the form to + # ssh://git@github.com/1234/repo.git or + # ssh://git@github.com:1234/user/repo.git + scp_url_m = explicit_url.match(%r{\A(.*@[^/:]+):(.+)\z}) + if Pathname.new(explicit_url).relative? && scp_url_m + # "^git@..." is also malformed as it is missing a scheme + # "ssh://git@..." is correct. + check_url = Addressable::URI.parse(scp_url_m[1]) + scheme = 'ssh://' unless check_url.scheme + + numbers_m = scp_url_m[2].split('/')[0].match(%r{\A[0-9]+\z}) + if numbers_m && numbers_m[0].to_i < 65_536 + # consider it an explicit-port URI, even though it's ambiguous. + explicit_url = Addressable::URI.parse(scheme + scp_url_m[1] + ':' + scp_url_m[2]) + else + explicit_url = Addressable::URI.parse(scheme + scp_url_m[1] + '/' + scp_url_m[2]) + PDK.logger.warn(_('--template-url appears to be an SCP-style url; it will be converted to an RFC-compliant URI: %{uri}') % { uri: explicit_url }) + end + end + explicit_uri = Addressable::URI.parse(uri_safe_location(explicit_url)) + explicit_uri.fragment = explicit_ref + else + explicit_uri = nil + end + metadata_uri = if PDK::Util.module_root && File.file?(File.join(module_root, 'metadata.json')) + Addressable::URI.parse(uri_safe(PDK::Util.module_metadata['template-url'])) + else + nil + end + answers_uri = if PDK.answers['template-url'] == 'https://github.com/puppetlabs/pdk-module-template' + # use the new github template-url if it is still the old one. + default_template_uri + elsif PDK.answers['template-url'] + Addressable::URI.parse(uri_safe(PDK.answers['template-url'])) + else + nil + end + default_uri = default_template_uri + + ary = [] + ary << { type: _('--template-url'), uri: explicit_uri, allow_fallback: false } + ary << { type: _('metadata.json'), uri: metadata_uri, allow_fallback: true } if metadata_uri + ary << { type: _('PDK answers'), uri: answers_uri, allow_fallback: true } unless metadata_uri + ary << { type: _('default'), uri: default_uri, allow_fallback: false } + ary + end + + # @returns Addressable::URI + def default_template_uri + if package_install? + Addressable::URI.new(scheme: 'file', host: '', path: File.join(package_cachedir, 'pdk-templates.git')) + else + Addressable::URI.parse('https://github.com/puppetlabs/pdk-templates') + end + end + + # @returns String + def default_template_ref + if PDK::Util.development_mode? + 'master' + else + PDK::TEMPLATE_REF + end + end + + end + end +end From 8a4e6dc3c4757466ac6cb34ca01e6fc90a535786 Mon Sep 17 00:00:00 2001 From: Hunter Haugen Date: Fri, 8 Jun 2018 16:18:19 -0700 Subject: [PATCH 03/24] (WIP) More changes today --- lib/pdk/module/update.rb | 8 ++++---- lib/pdk/util/template_uri.rb | 20 +++++++++++--------- 2 files changed, 15 insertions(+), 13 deletions(-) diff --git a/lib/pdk/module/update.rb b/lib/pdk/module/update.rb index 82334f73c..23ff3a7ce 100644 --- a/lib/pdk/module/update.rb +++ b/lib/pdk/module/update.rb @@ -86,7 +86,7 @@ def describe_ref_to_s(describe_ref) end def new_template_version - metadata_template_uri.fragment || PDK::Util.default_template_ref + metadata_template_uri.git_ref end def fetch_remote_version(template_ref) @@ -94,11 +94,11 @@ def fetch_remote_version(template_ref) return template_ref if template_ref == PDK::TEMPLATE_REF sha_length = GIT_DESCRIBE_PATTERN.match(current_template_version)[:sha].length - 1 - "#{template_ref}@#{PDK::Util::Git.ls_remote(PDK::Util.template_url(metadata_template_uri), template_ref)[0..sha_length]}" + "#{template_ref}@#{PDK::Util::Git.ls_remote(metadata_template_uri.location, template_ref)[0..sha_length]}" end def update_message - format_string = if metadata_template_uri == PDK::Util.default_template_uri + format_string = if metadata_template_uri == PDK::Util::TemplateURI.default_template_uri _('Updating %{module_name} using the default template, from %{current_version} to %{new_version}') else _('Updating %{module_name} using the template at %{template_url}, from %{current_version} to %{new_version}') @@ -106,7 +106,7 @@ def update_message format_string % { module_name: module_metadata.data['name'], - template_url: PDK::Util.template_url(metadata_template_uri), + template_url: metadata_template_uri.location, current_version: current_version, new_version: new_version, } diff --git a/lib/pdk/util/template_uri.rb b/lib/pdk/util/template_uri.rb index ed0f8073f..f43413897 100644 --- a/lib/pdk/util/template_uri.rb +++ b/lib/pdk/util/template_uri.rb @@ -30,6 +30,8 @@ def initialize(opts_or_uri) rescue Addressable::URI::InvalidURIError raise PDK::CLI::FatalError, _('PDK::Util::TemplateURI attempted initialization with a non-uri string: {string}') % { string: opts_or_uri } end + elsif opts_or_uri.is_a?(Addressable::URI) + opts_or_uri.dup else first_valid_uri(templates(opts_or_uri)) end @@ -71,6 +73,15 @@ def git_ref end end + # @returns PDK::Util::TemplateURI + def self.default_template_uri + if package_install? + PDK::Util::TemplateURI.new(Addressable::URI.new(scheme: 'file', host: '', path: File.join(package_cachedir, 'pdk-templates.git'))) + else + PDK::Util::TemplateURI.new('https://github.com/puppetlabs/pdk-templates') + end + end + private # `C:...` urls are not URI-safe. They should be of the form `/C:...` to @@ -191,15 +202,6 @@ def templates(opts) ary end - # @returns Addressable::URI - def default_template_uri - if package_install? - Addressable::URI.new(scheme: 'file', host: '', path: File.join(package_cachedir, 'pdk-templates.git')) - else - Addressable::URI.parse('https://github.com/puppetlabs/pdk-templates') - end - end - # @returns String def default_template_ref if PDK::Util.development_mode? From 4c22d553fe54777df0003b0bda92171856901795 Mon Sep 17 00:00:00 2001 From: Hunter Haugen Date: Sun, 10 Jun 2018 14:59:10 -0700 Subject: [PATCH 04/24] (WIP) Rename things better, start on specs --- lib/pdk/cli.rb | 2 +- lib/pdk/generate/module.rb | 4 +- lib/pdk/module/templatedir.rb | 4 +- lib/pdk/module/update.rb | 4 +- lib/pdk/util/template_uri.rb | 42 +-- spec/spec_helper_acceptance.rb | 2 +- spec/unit/pdk/util/template_uri_spec.rb | 280 ++++++++++++++++++++ spec/unit/pdk/util_spec.rb | 334 ------------------------ 8 files changed, 309 insertions(+), 363 deletions(-) create mode 100644 spec/unit/pdk/util/template_uri_spec.rb diff --git a/lib/pdk/cli.rb b/lib/pdk/cli.rb index 70cd160c5..d642e4d68 100644 --- a/lib/pdk/cli.rb +++ b/lib/pdk/cli.rb @@ -39,7 +39,7 @@ def self.run(args) end def self.template_url_option(dsl) - desc = _('Specifies the URL to the template to use when creating new modules or classes. (default: %{default_url})') % { default_url: PDK::Util.default_template_uri } + desc = _('Specifies the URL to the template to use when creating new modules or classes. (default: %{default_url})') % { default_url: PDK::Util::TemplateURI.default_template_uri } dsl.option nil, 'template-url', desc, argument: :required end diff --git a/lib/pdk/generate/module.rb b/lib/pdk/generate/module.rb index 3cb4126ae..8e11bcfb1 100644 --- a/lib/pdk/generate/module.rb +++ b/lib/pdk/generate/module.rb @@ -75,7 +75,7 @@ def self.invoke(opts = {}) end # Only update the answers files after metadata has been written. - if template_uri == PDK::Util.default_template_uri + if template_uri == PDK::Util::TemplateURI.default_template_uri # If the user specifies our default template url via the command # line, remove the saved template-url answer so that the template_uri # resolution can find new default URLs in the future. @@ -83,7 +83,7 @@ def self.invoke(opts = {}) else # Save the template-url answers if the module was generated using a # template/reference other than ours. - PDK.answers.update!('template-url' => template_uri) + PDK.answers.update!('template-url' => template_uri.metadata_format) end begin diff --git a/lib/pdk/module/templatedir.rb b/lib/pdk/module/templatedir.rb index 460d92202..a4e89f8af 100644 --- a/lib/pdk/module/templatedir.rb +++ b/lib/pdk/module/templatedir.rb @@ -45,7 +45,7 @@ def initialize(uri, module_metadata = {}, init = false) raise ArgumentError, _('PDK::Module::TemplateDir.new must be initialized with a PDK::Util::TemplateURI, got a %{uri_type}') % { uri_type: uri.class } end - if PDK::Util::Git.repo?(uri.location) + if PDK::Util::Git.repo?(uri.git_remote) # This is either a bare local repo or a remote. either way it needs cloning. @path = self.class.clone_template_repo(uri) temp_dir_clone = true @@ -57,7 +57,7 @@ def initialize(uri, module_metadata = {}, init = false) # cases or user un-expectations. PDK.logger.warn _("Repository '%{repo}' has a work-tree; skipping git reset.") % { repo: @path } end - @cloned_from = PDK::Util.deuri_path(uri.to_s) + @cloned_from = uri.metadata_format @init = init @moduleroot_dir = File.join(@path, 'moduleroot') diff --git a/lib/pdk/module/update.rb b/lib/pdk/module/update.rb index 23ff3a7ce..12a9a3171 100644 --- a/lib/pdk/module/update.rb +++ b/lib/pdk/module/update.rb @@ -94,7 +94,7 @@ def fetch_remote_version(template_ref) return template_ref if template_ref == PDK::TEMPLATE_REF sha_length = GIT_DESCRIBE_PATTERN.match(current_template_version)[:sha].length - 1 - "#{template_ref}@#{PDK::Util::Git.ls_remote(metadata_template_uri.location, template_ref)[0..sha_length]}" + "#{template_ref}@#{PDK::Util::Git.ls_remote(metadata_template_uri.git_remote, template_ref)[0..sha_length]}" end def update_message @@ -106,7 +106,7 @@ def update_message format_string % { module_name: module_metadata.data['name'], - template_url: metadata_template_uri.location, + template_url: metadata_template_uri.git_remote, current_version: current_version, new_version: new_version, } diff --git a/lib/pdk/util/template_uri.rb b/lib/pdk/util/template_uri.rb index f43413897..506695ee6 100644 --- a/lib/pdk/util/template_uri.rb +++ b/lib/pdk/util/template_uri.rb @@ -24,7 +24,7 @@ class TemplateURI # def initialize(opts_or_uri) # If a uri string is passed, skip the valid uri finding code. - @uri = if opts_or_uri.is_a?(String) + @uri = if opts_or_uri.is_a?(String) || opts_or_uri.is_a?(self.class) begin Addressable::URI.parse(opts_or_uri) rescue Addressable::URI::InvalidURIError @@ -33,7 +33,7 @@ def initialize(opts_or_uri) elsif opts_or_uri.is_a?(Addressable::URI) opts_or_uri.dup else - first_valid_uri(templates(opts_or_uri)) + first_valid_uri(self.class.templates(opts_or_uri)) end end @@ -41,27 +41,27 @@ def initialize(opts_or_uri) # metadata. # # @returns String - def url - human_readable(@uri.to_s) + def metadata_format + self.class.human_readable(@uri.to_s) end - alias to_s url + alias to_s metadata_format + alias to_str metadata_format - # This is the url without a fragment, suitable for git clones. I couldn't - # think of a better name. + # This is the url without a fragment, suitable for git clones. # # @returns String - def location + def git_remote if @uri.is_a?(Addressable::URI) && @uri.fragment - human_readable(@uri.to_s.chomp('#' + @uri.fragment)) + self.class.human_readable(@uri.to_s.chomp('#' + @uri.fragment)) else - human_readable(@uri.to_s) + self.class.human_readable(@uri.to_s) end end # This is the path of the URI, suitable for accessing directly from the shell. # @returns String def shell_path - human_readable(@uri.path) + self.class.human_readable(@uri.path) end # @returns String @@ -69,13 +69,13 @@ def git_ref if @uri.fragment @uri.fragment else - default_template_ref + self.class.default_template_ref end end # @returns PDK::Util::TemplateURI def self.default_template_uri - if package_install? + if PDK::Util.package_install? PDK::Util::TemplateURI.new(Addressable::URI.new(scheme: 'file', host: '', path: File.join(package_cachedir, 'pdk-templates.git'))) else PDK::Util::TemplateURI.new('https://github.com/puppetlabs/pdk-templates') @@ -89,7 +89,7 @@ def self.default_template_uri # either but are not handled here. Should they be? # # @returns String - def uri_safe(string) + def self.uri_safe(string) (Gem.win_platform? && string =~ %r{^[a-zA-Z][\|:]}) ? "/#{string}" : string end @@ -98,7 +98,7 @@ def uri_safe(string) # passed value is left alone. # # @returns String - def human_readable(string) + def self.human_readable(string) (Gem.win_platform? && string =~ %r{^\/[a-zA-Z][\|:]}) ? string[1..-1] : string end @@ -136,7 +136,7 @@ def valid_template?(template) # the lookup process should proceed to the next template directory if # the template file is not in this template directory. # - def templates(opts) + def self.templates(opts) explicit_url = opts.fetch(:'template-url', nil) explicit_ref = opts.fetch(:'template-ref', nil) @@ -174,25 +174,25 @@ def templates(opts) PDK.logger.warn(_('--template-url appears to be an SCP-style url; it will be converted to an RFC-compliant URI: %{uri}') % { uri: explicit_url }) end end - explicit_uri = Addressable::URI.parse(uri_safe_location(explicit_url)) + explicit_uri = Addressable::URI.parse(uri_safe(explicit_url)) explicit_uri.fragment = explicit_ref else explicit_uri = nil end - metadata_uri = if PDK::Util.module_root && File.file?(File.join(module_root, 'metadata.json')) + metadata_uri = if PDK::Util.module_root && File.file?(File.join(PDK::Util.module_root, 'metadata.json')) Addressable::URI.parse(uri_safe(PDK::Util.module_metadata['template-url'])) else nil end answers_uri = if PDK.answers['template-url'] == 'https://github.com/puppetlabs/pdk-module-template' # use the new github template-url if it is still the old one. - default_template_uri + Addressable::URI.parse(default_template_uri) elsif PDK.answers['template-url'] Addressable::URI.parse(uri_safe(PDK.answers['template-url'])) else nil end - default_uri = default_template_uri + default_uri = Addressable::URI.parse(default_template_uri) ary = [] ary << { type: _('--template-url'), uri: explicit_uri, allow_fallback: false } @@ -203,7 +203,7 @@ def templates(opts) end # @returns String - def default_template_ref + def self.default_template_ref if PDK::Util.development_mode? 'master' else diff --git a/spec/spec_helper_acceptance.rb b/spec/spec_helper_acceptance.rb index 8eade458d..b1f2506f4 100644 --- a/spec/spec_helper_acceptance.rb +++ b/spec/spec_helper_acceptance.rb @@ -60,7 +60,7 @@ def execute_script(script) RSpec.configure do |c| c.before(:suite) do RSpec.configuration.template_dir = Dir.mktmpdir - output, status = Open3.capture2e('git', 'clone', '--bare', PDK::Util.default_template_uri, RSpec.configuration.template_dir) + output, status = Open3.capture2e('git', 'clone', '--bare', PDK::Util::TemplateURI.default_template_uri, RSpec.configuration.template_dir) raise "Failed to cache module template: #{output}" unless status.success? tempdir = Dir.mktmpdir diff --git a/spec/unit/pdk/util/template_uri_spec.rb b/spec/unit/pdk/util/template_uri_spec.rb new file mode 100644 index 000000000..b20444837 --- /dev/null +++ b/spec/unit/pdk/util/template_uri_spec.rb @@ -0,0 +1,280 @@ +require 'spec_helper' + +describe PDK::Util::TemplateURI do + before :all do + PDK.answers.update!('template-url' => nil) + end + + subject(:template_uri) do + described_class.new(opts_or_uri) + end + + describe '.new' do + context 'with a string' do + let(:opts_or_uri) { 'https://github.com/my/pdk-templates.git#custom' } + + it 'can return a string for storing' do + expect(template_uri.to_s).to eq('https://github.com/my/pdk-templates.git#custom') + end + end + context 'with an addressable::uri' do + let(:opts_or_uri) { Addressable::URI.parse('https://github.com/my/pdk-templates.git#custom') } + + it 'can return a string for storing' do + expect(template_uri.to_s).to eq('https://github.com/my/pdk-templates.git#custom') + end + end + context 'with options' do + let(:opts_or_uri) do + { + :'template-url' => 'https://github.com/my/pdk-templates.git', + :'template-ref' => 'custom', + } + end + + it 'can return a string for storing' do + allow(PDK::Util::Git).to receive(:repo?).with(anything).and_return(true) + expect(template_uri.to_s).to eq('https://github.com/my/pdk-templates.git#custom') + end + end + end + + describe '.git_remote' do + context 'when the uri has a fragment' do + let(:opts_or_uri) { 'https://github.com/my/pdk-templates.git#custom' } + + it 'returns just the url portion' do + expect(template_uri.git_remote).to eq 'https://github.com/my/pdk-templates.git' + end + end + + context 'when the uri has no fragment' do + let(:opts_or_uri) { 'https://github.com/my/pdk-templates.git' } + + it 'returns just the url portion' do + expect(template_uri.git_remote).to eq 'https://github.com/my/pdk-templates.git' + end + end + + context 'when the uri is an absolute path' do + context 'on linux' do + let(:opts_or_uri) { '/my/pdk-templates.git#custom' } + + it 'returns url portion' do + allow(Gem).to receive(:win_platform?).and_return(false) + expect(template_uri.git_remote).to eq '/my/pdk-templates.git' + end + end + context 'on windows' do + let(:opts_or_uri) { '/C:/my/pdk-templates.git#custom' } + + it 'returns url portion' do + allow(Gem).to receive(:win_platform?).and_return(true) + expect(template_uri.git_remote).to eq 'C:/my/pdk-templates.git' + end + end + end + end + + describe '.git_ref' do + context 'when the uri has a fragment' do + let(:opts_or_uri) { 'https://github.com/my/pdk-templates.git#custom' } + + it 'returns just the ref portion' do + expect(template_uri.git_ref).to eq 'custom' + end + end + + context 'when the uri has no fragment' do + let(:opts_or_uri) { 'https://github.com/my/pdk-templates.git' } + + it 'returns the default ref' do + expect(template_uri.git_ref).to eq described_class.default_template_ref + end + end + end + + describe '.shell_path' do + let(:uri) { Addressable::URI.parse(template_url) } + + context 'when the uri has a schema' do + context 'on linux' do + let(:template_url) { 'file:///my/pdk-templates.git#fragment' } + + it 'returns the path' do + allow(Gem).to receive(:win_platform?).and_return(false) + expect(described_class.template_path(uri)).to eq '/my/pdk-templates.git' + end + end + + context 'on windows' do + let(:template_url) { 'file:///C:/my/pdk-templates.git#fragment' } + + it 'returns the path' do + allow(Gem).to receive(:win_platform?).and_return(true) + expect(described_class.template_path(uri)).to eq 'C:/my/pdk-templates.git' + end + end + end + + context 'when the uri is just an absolute path' do + context 'on linux' do + let(:template_url) { '/my/pdk-templates.git#custom' } + + it 'returns url portion' do + allow(Gem).to receive(:win_platform?).and_return(false) + expect(described_class.template_url(uri)).to eq '/my/pdk-templates.git' + end + end + context 'on windows' do + let(:template_url) { '/C:/my/pdk-templates.git#custom' } + + it 'returns url portion' do + allow(Gem).to receive(:win_platform?).and_return(true) + expect(described_class.template_url(uri)).to eq 'C:/my/pdk-templates.git' + end + end + end + end + + describe '.template_uri' do + before :each do + allow(PDK::Util::Git).to receive(:repo?).with(anything).and_return(true) + allow(described_class).to receive(:module_root).and_return('/path/to/module') + end + context 'when passed no options' do + context 'and there are no metadata or answers' do + before :each do + PDK.answers.update!('template-url' => nil) + end + it 'returns the default template' do + expect(described_class.template_uri({})).to eq(described_class.default_template_uri) + end + end + context 'and there are only answers' do + before :each do + PDK.answers.update!('template-url' => 'answer-templates') + end + it 'returns the answers template' do + expect(described_class.template_uri({})).to eq(Addressable::URI.parse('answer-templates')) + end + + context 'and the answer file template is invalid' do + before(:each) do + allow(described_class).to receive(:valid_template?).with(anything).and_call_original + allow(described_class).to receive(:valid_template?).with(uri: anything, type: anything, allow_fallback: true).and_return(false) + end + + it 'returns the default template' do + expect(described_class.template_uri({})).to eq(described_class.default_template_uri) + end + end + end + context 'and there are metadata and answers' do + before :each do + PDK.answers.update!('template-url' => 'answer-templates') + end + it 'returns the metadata template' do + allow(PDK::Module::Metadata).to receive(:from_file).with('/path/to/module/metadata.json').and_return(mock_metadata) + allow(File).to receive(:file?).with('/path/to/module/metadata.json').and_return(true) + allow(File).to receive(:file?).with(%r{PDK_VERSION}).and_return(true) + expect(described_class.template_uri({})).to eq(Addressable::URI.parse('metadata-templates')) + end + end + end + context 'when there are metadata and answers' do + before :each do + PDK.answers.update!('template-url' => 'answer-templates') + allow(PDK::Module::Metadata).to receive(:from_file).with(File.join(described_class.module_root, 'metadata.json')).and_return(mock_metadata) + end + context 'and passed template-url' do + it 'returns the specified template' do + expect(described_class.template_uri(:'template-url' => 'cli-templates')).to eq(Addressable::URI.parse('cli-templates')) + end + end + context 'and passed windows template-url' do + it 'returns the specified template' do + allow(Gem).to receive(:win_platform?).and_return(true) + expect(described_class.template_uri(:'template-url' => 'C:\cli-templates')).to eq(Addressable::URI.parse('/C:\cli-templates')) + end + end + context 'and passed template-ref' do + it 'errors because it requires url with ref' do + expect { described_class.template_uri(:'template-ref' => 'cli-ref') }.to raise_error(PDK::CLI::FatalError, %r{--template-ref requires --template-url}) + end + end + context 'and passed template-url and template-ref' do + it 'returns the specified template and ref' do + uri = Addressable::URI.parse('cli-templates') + uri.fragment = 'cli-ref' + expect(described_class.template_uri(:'template-url' => 'cli-templates', :'template-ref' => 'cli-ref')).to eq(uri) + end + end + end + end + + describe '.default_template_uri' do + subject { described_class.default_template_uri } + + context 'when it is a package install' do + before(:each) do + allow(described_class).to receive(:package_install?).and_return(true) + end + + it 'returns the file template repo' do + allow(described_class).to receive(:package_cachedir).and_return('/path/to/pdk') + is_expected.to eq(Addressable::URI.parse('file:///path/to/pdk/pdk-templates.git')) + end + end + context 'when it is not a package install' do + before(:each) do + allow(described_class).to receive(:package_install?).and_return(false) + end + + it 'returns puppetlabs template url' do + is_expected.to eq(Addressable::URI.parse('https://github.com/puppetlabs/pdk-templates')) + end + end + end + + describe '.default_template_ref' do + subject { described_class.default_template_ref } + + context 'with a custom template repo' do + before(:each) do + allow(described_class).to receive(:default_template_url).and_return('custom_template_url') + end + + it 'returns master' do + is_expected.to eq('master') + end + end + + context 'with the default template repo' do + before(:each) do + allow(described_class).to receive(:default_template_url).and_return('puppetlabs_template_url') + end + + context 'not in development mode' do + before(:each) do + allow(described_class).to receive(:development_mode?).and_return(false) + end + + it 'returns the built-in TEMPLATE_REF' do + is_expected.to eq(PDK::TEMPLATE_REF) + end + end + + context 'in development mode' do + before(:each) do + allow(described_class).to receive(:development_mode?).and_return(true) + end + + it 'returns master' do + is_expected.to eq('master') + end + end + end + end + +end diff --git a/spec/unit/pdk/util_spec.rb b/spec/unit/pdk/util_spec.rb index c177f7299..a9221199e 100644 --- a/spec/unit/pdk/util_spec.rb +++ b/spec/unit/pdk/util_spec.rb @@ -363,340 +363,6 @@ end end - describe '.template_url' do - let(:uri) { Addressable::URI.parse(template_url) } - - context 'when the uri has a fragment' do - let(:template_url) { 'https://github.com/my/pdk-templates.git#custom' } - - it 'returns just the url portion' do - expect(described_class.template_url(uri)).to eq 'https://github.com/my/pdk-templates.git' - end - end - - context 'when the uri has no fragment' do - let(:template_url) { 'https://github.com/my/pdk-templates.git' } - - it 'returns just the url portion' do - expect(described_class.template_url(uri)).to eq 'https://github.com/my/pdk-templates.git' - end - end - - context 'when the uri is an absolute path' do - context 'on linux' do - let(:template_url) { '/my/pdk-templates.git#custom' } - - it 'returns url portion' do - allow(Gem).to receive(:win_platform?).and_return(false) - expect(described_class.template_url(uri)).to eq '/my/pdk-templates.git' - end - end - context 'on windows' do - let(:template_url) { '/C:/my/pdk-templates.git#custom' } - - it 'returns url portion' do - allow(Gem).to receive(:win_platform?).and_return(true) - expect(described_class.template_url(uri)).to eq 'C:/my/pdk-templates.git' - end - end - end - end - - describe '.template_ref' do - let(:uri) { Addressable::URI.parse(template_url) } - - context 'when the uri has a fragment' do - let(:template_url) { 'https://github.com/my/pdk-templates.git#custom' } - - it 'returns just the ref portion' do - expect(described_class.template_ref(uri)).to eq 'custom' - end - end - - context 'when the uri has no fragment' do - let(:template_url) { 'https://github.com/my/pdk-templates.git' } - - it 'returns the default ref' do - expect(described_class.template_ref(uri)).to eq described_class.default_template_ref - end - end - end - - describe '.template_path' do - let(:uri) { Addressable::URI.parse(template_url) } - - context 'when the uri has a schema' do - context 'on linux' do - let(:template_url) { 'file:///my/pdk-templates.git#fragment' } - - it 'returns the path' do - allow(Gem).to receive(:win_platform?).and_return(false) - expect(described_class.template_path(uri)).to eq '/my/pdk-templates.git' - end - end - - context 'on windows' do - let(:template_url) { 'file:///C:/my/pdk-templates.git#fragment' } - - it 'returns the path' do - allow(Gem).to receive(:win_platform?).and_return(true) - expect(described_class.template_path(uri)).to eq 'C:/my/pdk-templates.git' - end - end - end - - context 'when the uri is just an absolute path' do - context 'on linux' do - let(:template_url) { '/my/pdk-templates.git#custom' } - - it 'returns url portion' do - allow(Gem).to receive(:win_platform?).and_return(false) - expect(described_class.template_url(uri)).to eq '/my/pdk-templates.git' - end - end - context 'on windows' do - let(:template_url) { '/C:/my/pdk-templates.git#custom' } - - it 'returns url portion' do - allow(Gem).to receive(:win_platform?).and_return(true) - expect(described_class.template_url(uri)).to eq 'C:/my/pdk-templates.git' - end - end - end - end - - describe '.template_uri' do - before(:each) do - allow(PDK::Util::Git).to receive(:repo?).with(anything).and_return(true) - allow(described_class).to receive(:module_root).and_return('/path/to/module') - end - - context 'when passed no options' do - context 'and there are no metadata or answers' do - before(:each) do - PDK.answers.update!('template-url' => nil) - end - - it 'returns the default template' do - expect(described_class.template_uri({})).to eq(described_class.default_template_uri) - end - end - - context 'and there are only answers' do - before(:each) do - PDK.answers.update!('template-url' => 'answer-templates') - end - - it 'returns the answers template' do - expect(described_class.template_uri({})).to eq(Addressable::URI.parse('answer-templates')) - end - - context 'and the answer file template is invalid' do - before(:each) do - allow(described_class).to receive(:valid_template?).with(anything).and_call_original - allow(described_class).to receive(:valid_template?).with(uri: anything, allow_fallback: true, type: anything).and_return(false) - end - - it 'returns the default template' do - expect(described_class.template_uri({})).to eq(described_class.default_template_uri) - end - end - end - - context 'and there are metadata and answers' do - before(:each) do - PDK.answers.update!('template-url' => 'answer-templates') - end - - it 'returns the metadata template' do - allow(PDK::Module::Metadata).to receive(:from_file).with('/path/to/module/metadata.json').and_return(mock_metadata) - allow(File).to receive(:file?).with('/path/to/module/metadata.json').and_return(true) - allow(File).to receive(:file?).with(%r{PDK_VERSION}).and_return(true) - expect(described_class.template_uri({})).to eq(Addressable::URI.parse('metadata-templates')) - end - end - end - - context 'when there are metadata and answers' do - before(:each) do - PDK.answers.update!('template-url' => 'answer-templates') - allow(PDK::Module::Metadata).to receive(:from_file).with(File.join(described_class.module_root, 'metadata.json')).and_return(mock_metadata) - end - - context 'and passed template-url' do - it 'returns the specified template' do - expect(described_class.template_uri(:'template-url' => 'cli-templates')).to eq(Addressable::URI.parse('cli-templates')) - end - end - - context 'and passed windows template-url' do - it 'returns the specified template' do - allow(Gem).to receive(:win_platform?).and_return(true) - expect(described_class.template_uri(:'template-url' => 'C:\cli-templates')).to eq(Addressable::URI.parse('/C:\cli-templates')) - end - end - - context 'and passed template-ref' do - it 'errors because it requires url with ref' do - expect { described_class.template_uri(:'template-ref' => 'cli-ref') }.to raise_error(PDK::CLI::FatalError, %r{--template-ref requires --template-url}) - end - end - - context 'and passed template-url and template-ref' do - it 'returns the specified template and ref' do - uri = Addressable::URI.parse('cli-templates') - uri.fragment = 'cli-ref' - expect(described_class.template_uri(:'template-url' => 'cli-templates', :'template-ref' => 'cli-ref')).to eq(uri) - end - end - end - end - - describe '.valid_template?' do - subject { described_class.valid_template?(template) } - - context 'when template is nil' do - let(:template) { nil } - - it { is_expected.to be_falsey } - end - - context 'when template is a Hash' do - context 'and template[:uri] => nil' do - let(:template) { { uri: nil } } - - it { is_expected.to be_falsey } - end - - context 'and template[:path] => directory' do - let(:template) do - { - uri: Addressable::URI.parse('file:///a/template/on/disk'), - path: '/a/template/on/disk', - } - end - - before(:each) do - allow(Gem).to receive(:win_platform?).and_return(true) - allow(described_class).to receive(:package_install?).and_return(false) - allow(PDK::Util::Git).to receive(:repo?).with(template[:uri]).and_return(false) - end - - context 'that exists and contains a valid template' do - before(:each) do - allow(File).to receive(:directory?).with(template[:path]).and_return(true) - allow(File).to receive(:directory?).with(File.join(template[:path], 'moduleroot')).and_return(true) - allow(File).to receive(:directory?).with(File.join(template[:path], 'moduleroot_init')).and_return(true) - end - - it { is_expected.to be_truthy } - end - - context 'that exists and does not contain a valid template' do - before(:each) do - allow(File).to receive(:directory?).with(template[:path]).and_return(true) - allow(File).to receive(:directory?).with(File.join(template[:path], 'moduleroot')).and_return(false) - allow(File).to receive(:directory?).with(File.join(template[:path], 'moduleroot_init')).and_return(false) - end - - it { is_expected.to be_falsey } - end - - context 'that does not exist' do - before(:each) do - allow(File).to receive(:directory?).with(template[:path]).and_return(false) - end - - it { is_expected.to be_falsey } - end - end - - context 'and template[:uri] => URI' do - let(:template) { { uri: Addressable::URI.parse('https://this.is/a/repo.git') } } - - context 'that points to a git repo' do - before(:each) do - allow(PDK::Util::Git).to receive(:repo?).with(template[:uri]).and_return(true) - end - - it { is_expected.to be_truthy } - end - - context 'that does not point to a git repo' do - before(:each) do - allow(PDK::Util::Git).to receive(:repo?).with(template[:uri]).and_return(false) - end - - it { is_expected.to be_falsey } - end - end - end - end - - describe '.default_template_uri' do - subject { described_class.default_template_uri } - - context 'when it is a package install' do - before(:each) do - allow(described_class).to receive(:package_install?).and_return(true) - end - - it 'returns the file template repo' do - allow(described_class).to receive(:package_cachedir).and_return('/path/to/pdk') - is_expected.to eq(Addressable::URI.parse('file:///path/to/pdk/pdk-templates.git')) - end - end - context 'when it is not a package install' do - before(:each) do - allow(described_class).to receive(:package_install?).and_return(false) - end - - it 'returns puppetlabs template url' do - is_expected.to eq(Addressable::URI.parse('https://github.com/puppetlabs/pdk-templates')) - end - end - end - - describe '.default_template_ref' do - subject { described_class.default_template_ref } - - context 'with a custom template repo' do - before(:each) do - allow(described_class).to receive(:default_template_url).and_return('custom_template_url') - end - - it 'returns master' do - is_expected.to eq('master') - end - end - - context 'with the default template repo' do - before(:each) do - allow(described_class).to receive(:default_template_url).and_return('puppetlabs_template_url') - end - - context 'not in development mode' do - before(:each) do - allow(described_class).to receive(:development_mode?).and_return(false) - end - - it 'returns the built-in TEMPLATE_REF' do - is_expected.to eq(PDK::TEMPLATE_REF) - end - end - - context 'in development mode' do - before(:each) do - allow(described_class).to receive(:development_mode?).and_return(true) - end - - it 'returns master' do - is_expected.to eq('master') - end - end - end - end - describe 'module_metadata' do subject(:result) { described_class.module_metadata } From 741fc4837d3d7413e8b12f3a44ce4027390ff851 Mon Sep 17 00:00:00 2001 From: Hunter Haugen Date: Tue, 12 Jun 2018 09:38:23 -0700 Subject: [PATCH 05/24] Specs for shell_path --- spec/unit/pdk/util/template_uri_spec.rb | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/spec/unit/pdk/util/template_uri_spec.rb b/spec/unit/pdk/util/template_uri_spec.rb index b20444837..a9f077165 100644 --- a/spec/unit/pdk/util/template_uri_spec.rb +++ b/spec/unit/pdk/util/template_uri_spec.rb @@ -95,43 +95,41 @@ end describe '.shell_path' do - let(:uri) { Addressable::URI.parse(template_url) } - context 'when the uri has a schema' do context 'on linux' do - let(:template_url) { 'file:///my/pdk-templates.git#fragment' } + let(:opts_or_uri) { 'file:///my/pdk-templates.git#fragment' } it 'returns the path' do allow(Gem).to receive(:win_platform?).and_return(false) - expect(described_class.template_path(uri)).to eq '/my/pdk-templates.git' + expect(template_uri.shell_path).to eq '/my/pdk-templates.git' end end context 'on windows' do - let(:template_url) { 'file:///C:/my/pdk-templates.git#fragment' } + let(:opts_or_uri) { 'file:///C:/my/pdk-templates.git#fragment' } it 'returns the path' do allow(Gem).to receive(:win_platform?).and_return(true) - expect(described_class.template_path(uri)).to eq 'C:/my/pdk-templates.git' + expect(template_uri.shell_path).to eq 'C:/my/pdk-templates.git' end end end context 'when the uri is just an absolute path' do context 'on linux' do - let(:template_url) { '/my/pdk-templates.git#custom' } + let(:opts_or_uri) { '/my/pdk-templates.git#custom' } it 'returns url portion' do allow(Gem).to receive(:win_platform?).and_return(false) - expect(described_class.template_url(uri)).to eq '/my/pdk-templates.git' + expect(template_uri.shell_path).to eq '/my/pdk-templates.git' end end context 'on windows' do - let(:template_url) { '/C:/my/pdk-templates.git#custom' } + let(:opts_or_uri) { '/C:/my/pdk-templates.git#custom' } it 'returns url portion' do allow(Gem).to receive(:win_platform?).and_return(true) - expect(described_class.template_url(uri)).to eq 'C:/my/pdk-templates.git' + expect(template_uri.shell_path).to eq 'C:/my/pdk-templates.git' end end end From 89fba472d382d545bd77058f357888044702cae5 Mon Sep 17 00:00:00 2001 From: Hunter Haugen Date: Tue, 12 Jun 2018 10:02:57 -0700 Subject: [PATCH 06/24] More specs --- spec/unit/pdk/util/template_uri_spec.rb | 179 ++++++++++++++---------- 1 file changed, 103 insertions(+), 76 deletions(-) diff --git a/spec/unit/pdk/util/template_uri_spec.rb b/spec/unit/pdk/util/template_uri_spec.rb index a9f077165..be33a3ab0 100644 --- a/spec/unit/pdk/util/template_uri_spec.rb +++ b/spec/unit/pdk/util/template_uri_spec.rb @@ -37,6 +37,109 @@ expect(template_uri.to_s).to eq('https://github.com/my/pdk-templates.git#custom') end end + context 'combinations of answers, options, and defaults' do + before :each do + allow(PDK::Util::Git).to receive(:repo?).with(anything).and_return(true) + allow(PDK::Util).to receive(:module_root).and_return('/path/to/module') + allow(PDK::Util).to receive(:development_mode?).and_return(false) + end + + let(:pdk_version) { '1.2.3' } + let(:template_url) { 'metadata-templates' } + let(:template_ref) { nil } + let(:mock_metadata) do + instance_double( + PDK::Module::Metadata, + data: { + 'pdk-version' => pdk_version, + 'template-url' => template_url, + 'template-ref' => template_ref, + }, + ) + end + + let(:opts_or_uri) { {} } + + context 'when passed no options' do + context 'and there are no metadata or answers' do + before :each do + PDK.answers.update!('template-url' => nil) + end + it 'returns the default template' do + expect(template_uri.to_s).to eq(described_class.default_template_uri.to_s) + end + end + context 'and there are only answers' do + before :each do + PDK.answers.update!('template-url' => 'answer-templates') + end + it 'returns the answers template' do + expect(template_uri.to_s).to eq('answer-templates') + end + + context 'and the answer file template is invalid' do + before(:each) do + allow(template_uri).to receive(:valid_template?).with(anything).and_call_original + allow(template_uri).to receive(:valid_template?).with(uri: anything, type: anything, allow_fallback: true).and_return(false) + end + + it 'returns the default template' do + pending + expect(template_uri.to_s).to eq(described_class.default_template_uri.to_s) + end + end + end + context 'and there are metadata and answers' do + before :each do + PDK.answers.update!('template-url' => 'answer-templates') + end + it 'returns the metadata template' do + allow(PDK::Module::Metadata).to receive(:from_file).with('/path/to/module/metadata.json').and_return(mock_metadata) + allow(File).to receive(:file?).with('/path/to/module/metadata.json').and_return(true) + allow(File).to receive(:file?).with(%r{PDK_VERSION}).and_return(true) + expect(template_uri.to_s).to eq('metadata-templates') + end + end + end + context 'when there are metadata and answers' do + before :each do + PDK.answers.update!('template-url' => 'answer-templates') + allow(PDK::Module::Metadata).to receive(:from_file).with(File.join(PDK::Util.module_root, 'metadata.json')).and_return(mock_metadata) + end + + context 'and passed template-url' do + let(:opts_or_uri) { { :'template-url' => 'cli-templates' } } + + it 'returns the specified template' do + expect(template_uri.to_s).to eq('cli-templates') + end + end + context 'and passed windows template-url' do + let(:opts_or_uri) { { :'template-url' => 'C:\cli-templates' } } + + it 'returns the specified template' do + allow(Gem).to receive(:win_platform?).and_return(true) + expect(template_uri.to_s).to eq('C:\cli-templates') + end + end + context 'and passed template-ref' do + let(:opts_or_uri) { { :'template-ref' => 'cli-ref' } } + + it 'errors because it requires url with ref' do + expect { template_uri }.to raise_error(PDK::CLI::FatalError, %r{--template-ref requires --template-url}) + end + end + context 'and passed template-url and template-ref' do + let(:opts_or_uri) { { :'template-url' => 'cli-templates', :'template-ref' => 'cli-ref' } } + + it 'returns the specified template and ref' do + uri = Addressable::URI.parse('cli-templates') + uri.fragment = 'cli-ref' + expect(template_uri.to_s).to eq(uri.to_s) + end + end + end + end end describe '.git_remote' do @@ -135,82 +238,6 @@ end end - describe '.template_uri' do - before :each do - allow(PDK::Util::Git).to receive(:repo?).with(anything).and_return(true) - allow(described_class).to receive(:module_root).and_return('/path/to/module') - end - context 'when passed no options' do - context 'and there are no metadata or answers' do - before :each do - PDK.answers.update!('template-url' => nil) - end - it 'returns the default template' do - expect(described_class.template_uri({})).to eq(described_class.default_template_uri) - end - end - context 'and there are only answers' do - before :each do - PDK.answers.update!('template-url' => 'answer-templates') - end - it 'returns the answers template' do - expect(described_class.template_uri({})).to eq(Addressable::URI.parse('answer-templates')) - end - - context 'and the answer file template is invalid' do - before(:each) do - allow(described_class).to receive(:valid_template?).with(anything).and_call_original - allow(described_class).to receive(:valid_template?).with(uri: anything, type: anything, allow_fallback: true).and_return(false) - end - - it 'returns the default template' do - expect(described_class.template_uri({})).to eq(described_class.default_template_uri) - end - end - end - context 'and there are metadata and answers' do - before :each do - PDK.answers.update!('template-url' => 'answer-templates') - end - it 'returns the metadata template' do - allow(PDK::Module::Metadata).to receive(:from_file).with('/path/to/module/metadata.json').and_return(mock_metadata) - allow(File).to receive(:file?).with('/path/to/module/metadata.json').and_return(true) - allow(File).to receive(:file?).with(%r{PDK_VERSION}).and_return(true) - expect(described_class.template_uri({})).to eq(Addressable::URI.parse('metadata-templates')) - end - end - end - context 'when there are metadata and answers' do - before :each do - PDK.answers.update!('template-url' => 'answer-templates') - allow(PDK::Module::Metadata).to receive(:from_file).with(File.join(described_class.module_root, 'metadata.json')).and_return(mock_metadata) - end - context 'and passed template-url' do - it 'returns the specified template' do - expect(described_class.template_uri(:'template-url' => 'cli-templates')).to eq(Addressable::URI.parse('cli-templates')) - end - end - context 'and passed windows template-url' do - it 'returns the specified template' do - allow(Gem).to receive(:win_platform?).and_return(true) - expect(described_class.template_uri(:'template-url' => 'C:\cli-templates')).to eq(Addressable::URI.parse('/C:\cli-templates')) - end - end - context 'and passed template-ref' do - it 'errors because it requires url with ref' do - expect { described_class.template_uri(:'template-ref' => 'cli-ref') }.to raise_error(PDK::CLI::FatalError, %r{--template-ref requires --template-url}) - end - end - context 'and passed template-url and template-ref' do - it 'returns the specified template and ref' do - uri = Addressable::URI.parse('cli-templates') - uri.fragment = 'cli-ref' - expect(described_class.template_uri(:'template-url' => 'cli-templates', :'template-ref' => 'cli-ref')).to eq(uri) - end - end - end - end - describe '.default_template_uri' do subject { described_class.default_template_uri } From 78c507c5889e35e1139ca457fec871b18177ca1b Mon Sep 17 00:00:00 2001 From: Hunter Haugen Date: Tue, 12 Jun 2018 11:08:15 -0700 Subject: [PATCH 07/24] Final specs --- lib/pdk/util/template_uri.rb | 2 +- spec/unit/pdk/util/template_uri_spec.rb | 16 ++++++++-------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/lib/pdk/util/template_uri.rb b/lib/pdk/util/template_uri.rb index 506695ee6..5d19ded30 100644 --- a/lib/pdk/util/template_uri.rb +++ b/lib/pdk/util/template_uri.rb @@ -76,7 +76,7 @@ def git_ref # @returns PDK::Util::TemplateURI def self.default_template_uri if PDK::Util.package_install? - PDK::Util::TemplateURI.new(Addressable::URI.new(scheme: 'file', host: '', path: File.join(package_cachedir, 'pdk-templates.git'))) + PDK::Util::TemplateURI.new(Addressable::URI.new(scheme: 'file', host: '', path: File.join(PDK::Util.package_cachedir, 'pdk-templates.git'))) else PDK::Util::TemplateURI.new('https://github.com/puppetlabs/pdk-templates') end diff --git a/spec/unit/pdk/util/template_uri_spec.rb b/spec/unit/pdk/util/template_uri_spec.rb index be33a3ab0..5b3213dd6 100644 --- a/spec/unit/pdk/util/template_uri_spec.rb +++ b/spec/unit/pdk/util/template_uri_spec.rb @@ -239,25 +239,25 @@ end describe '.default_template_uri' do - subject { described_class.default_template_uri } + subject(:default_uri) { described_class.default_template_uri } context 'when it is a package install' do before(:each) do - allow(described_class).to receive(:package_install?).and_return(true) + allow(PDK::Util).to receive(:package_install?).and_return(true) end it 'returns the file template repo' do - allow(described_class).to receive(:package_cachedir).and_return('/path/to/pdk') - is_expected.to eq(Addressable::URI.parse('file:///path/to/pdk/pdk-templates.git')) + allow(PDK::Util).to receive(:package_cachedir).and_return('/path/to/pdk') + expect(default_uri.to_s).to eq('file:///path/to/pdk/pdk-templates.git') end end context 'when it is not a package install' do before(:each) do - allow(described_class).to receive(:package_install?).and_return(false) + allow(PDK::Util).to receive(:package_install?).and_return(false) end it 'returns puppetlabs template url' do - is_expected.to eq(Addressable::URI.parse('https://github.com/puppetlabs/pdk-templates')) + expect(default_uri.to_s).to eq('https://github.com/puppetlabs/pdk-templates') end end end @@ -282,7 +282,7 @@ context 'not in development mode' do before(:each) do - allow(described_class).to receive(:development_mode?).and_return(false) + allow(PDK::Util).to receive(:development_mode?).and_return(false) end it 'returns the built-in TEMPLATE_REF' do @@ -292,7 +292,7 @@ context 'in development mode' do before(:each) do - allow(described_class).to receive(:development_mode?).and_return(true) + allow(PDK::Util).to receive(:development_mode?).and_return(true) end it 'returns master' do From 0a9c4ffaa074a3c902d494650961a856fb0597d6 Mon Sep 17 00:00:00 2001 From: Hunter Haugen Date: Fri, 22 Jun 2018 11:11:52 -0700 Subject: [PATCH 08/24] Fix that I missed --- lib/pdk/module/templatedir.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/pdk/module/templatedir.rb b/lib/pdk/module/templatedir.rb index a4e89f8af..d5a9eb5c2 100644 --- a/lib/pdk/module/templatedir.rb +++ b/lib/pdk/module/templatedir.rb @@ -305,8 +305,8 @@ def self.clone_template_repo(uri) # template repo in `%AppData%` or `$XDG_CACHE_DIR` and update before # use. temp_dir = PDK::Util.make_tmpdir_name('pdk-templates') - origin_repo = PDK::Util.template_url(uri) - git_ref = PDK::Util.template_ref(uri) + origin_repo = uri.git_remote + git_ref = uri.git_ref clone_result = PDK::Util::Git.git('clone', origin_repo, temp_dir) From c7321b28269d637aaf316b3cf22c425f6fe8568f Mon Sep 17 00:00:00 2001 From: Tim Sharpe Date: Thu, 17 Jan 2019 15:26:42 +1100 Subject: [PATCH 09/24] Fix spec failures --- lib/pdk/generate/module.rb | 4 +- lib/pdk/generate/puppet_object.rb | 4 +- lib/pdk/util/template_uri.rb | 61 ++++++++++---------- spec/unit/pdk/generate/module_spec.rb | 8 ++- spec/unit/pdk/generate/puppet_object_spec.rb | 2 +- spec/unit/pdk/module/convert_spec.rb | 21 +++++-- spec/unit/pdk/module/template_dir_spec.rb | 24 ++++---- spec/unit/pdk/module/update_spec.rb | 4 +- spec/unit/pdk/util/template_uri_spec.rb | 5 +- 9 files changed, 77 insertions(+), 56 deletions(-) diff --git a/lib/pdk/generate/module.rb b/lib/pdk/generate/module.rb index 8e11bcfb1..f08165898 100644 --- a/lib/pdk/generate/module.rb +++ b/lib/pdk/generate/module.rb @@ -75,7 +75,7 @@ def self.invoke(opts = {}) end # Only update the answers files after metadata has been written. - if template_uri == PDK::Util::TemplateURI.default_template_uri + if template_uri.metadata_format == PDK::Util::TemplateURI.default_template_uri.metadata_format # If the user specifies our default template url via the command # line, remove the saved template-url answer so that the template_uri # resolution can find new default URLs in the future. @@ -90,7 +90,7 @@ def self.invoke(opts = {}) if FileUtils.mv(temp_target_dir, target_dir) Dir.chdir(target_dir) { PDK::Util::Bundler.ensure_bundle! } unless opts[:'skip-bundle-install'] - PDK.logger.info _('Module \'%{name}\' generated at path \'%{path}\', from template \'%{url}\'.') % { name: opts[:module_name], path: target_dir, url: PDK::Util.template_url(template_uri) } + PDK.logger.info _('Module \'%{name}\' generated at path \'%{path}\', from template \'%{url}\'.') % { name: opts[:module_name], path: target_dir, url: template_uri.shell_path } PDK.logger.info(_('In your module directory, add classes with the \'pdk new class\' command.')) end rescue Errno::EACCES => e diff --git a/lib/pdk/generate/puppet_object.rb b/lib/pdk/generate/puppet_object.rb index a06948cfc..c7207714e 100644 --- a/lib/pdk/generate/puppet_object.rb +++ b/lib/pdk/generate/puppet_object.rb @@ -210,7 +210,7 @@ def with_templates next end - PDK::Module::TemplateDir.new(template[:uri]) do |template_dir| + PDK::Module::TemplateDir.new(PDK::Util::TemplateURI.new(template[:uri])) do |template_dir| template_paths = template_dir.object_template_for(object_type) if template_paths @@ -250,7 +250,7 @@ def with_templates # # @api private def templates - @templates ||= PDK::Util.templates(@options) + @templates ||= PDK::Util::TemplateURI.templates(@options) end # Retrieves the name of the module (without the forge username) from the diff --git a/lib/pdk/util/template_uri.rb b/lib/pdk/util/template_uri.rb index 5d19ded30..0c24d4545 100644 --- a/lib/pdk/util/template_uri.rb +++ b/lib/pdk/util/template_uri.rb @@ -37,6 +37,10 @@ def initialize(opts_or_uri) end end + def ==(other) + @uri == other.uri + end + # This is the URI represented in a format suitable for writing to # metadata. # @@ -82,8 +86,6 @@ def self.default_template_uri end end - private - # `C:...` urls are not URI-safe. They should be of the form `/C:...` to # be URI-safe. scp-like urls like `user@host:/path` are not URI-safe # either but are not handled here. Should they be? @@ -102,33 +104,6 @@ def self.human_readable(string) (Gem.win_platform? && string =~ %r{^\/[a-zA-Z][\|:]}) ? string[1..-1] : string end - # @returns Addressable::URI - def first_valid_uri(templates_array) - # 1. Get the four sources of URIs - # 2. Pick the first non-nil URI - # 3. Error if the URI is not a valid git repo (missing directory or http 404) - # 4. Leave updating answers/metadata to other code - found_template = templates_array.find { |t| valid_template?(t) } - - raise PDK::CLI::FatalError, _('Unable to find a valid module template to use.') if found_template.nil? - found_template[:uri] - end - - def valid_template?(template) - return false if template.nil? - return false if template[:uri].nil? - - if template[:path] && File.directory?(template[:path]) - PDK::Module::TemplateDir.new(template[:uri]) {} - return true - end - return true if PDK::Util::Git.repo?(template[:uri]) - - false - rescue ArgumentError - return false - end - # @return [Array Object}>] an array of hashes. Each hash # contains 3 keys: :type contains a String that describes the template # directory, :url contains a String with the URL to the template @@ -211,6 +186,34 @@ def self.default_template_ref end end + private + + # @returns Addressable::URI + def first_valid_uri(templates_array) + # 1. Get the four sources of URIs + # 2. Pick the first non-nil URI + # 3. Error if the URI is not a valid git repo (missing directory or http 404) + # 4. Leave updating answers/metadata to other code + found_template = templates_array.find { |t| valid_template?(t) } + + raise PDK::CLI::FatalError, _('Unable to find a valid module template to use.') if found_template.nil? + found_template[:uri] + end + + def valid_template?(template) + return false if template.nil? + return false if template[:uri].nil? + + if template[:path] && File.directory?(template[:path]) + PDK::Module::TemplateDir.new(template[:uri]) {} + return true + end + return true if PDK::Util::Git.repo?(template[:uri].to_s) + + false + rescue ArgumentError + return false + end end end end diff --git a/spec/unit/pdk/generate/module_spec.rb b/spec/unit/pdk/generate/module_spec.rb index 1583c9009..30357d03f 100644 --- a/spec/unit/pdk/generate/module_spec.rb +++ b/spec/unit/pdk/generate/module_spec.rb @@ -198,14 +198,12 @@ end it 'saves the template-url and template-ref to the answer file if it is not the default template' do - expect(PDK::Util).to receive(:template_uri).and_return(Addressable::URI.parse('cli-template')) expect(PDK.answers).to receive(:update!).with('template-url' => Addressable::URI.parse('cli-template')) described_class.invoke(invoke_opts.merge(:'template-url' => 'cli-template')) end it 'clears the saved template-url answer if it is the default template' do - expect(PDK::Util).to receive(:template_uri).and_return(Addressable::URI.parse('https://github.com/puppetlabs/pdk-templates')) expect(PDK.answers).to receive(:update!).with('template-url' => nil).and_call_original described_class.invoke(invoke_opts.merge(:'template-url' => 'https://github.com/puppetlabs/pdk-templates')) @@ -246,8 +244,12 @@ end context 'and pdk is not installed from packages' do + before(:each) do + allow(PDK::Util).to receive(:package_install?).and_return(false) + end + it 'uses the default template to generate the module' do - expect(PDK::Module::TemplateDir).to receive(:new).with(PDK::Util.default_template_uri, anything, anything).and_yield(test_template_dir) + expect(PDK::Module::TemplateDir).to receive(:new).with(any_args).and_yield(test_template_dir) expect(PDK.answers).not_to receive(:update!).with(:'template-url' => anything) described_class.invoke(invoke_opts) diff --git a/spec/unit/pdk/generate/puppet_object_spec.rb b/spec/unit/pdk/generate/puppet_object_spec.rb index 87bbf6c31..be44f2668 100644 --- a/spec/unit/pdk/generate/puppet_object_spec.rb +++ b/spec/unit/pdk/generate/puppet_object_spec.rb @@ -142,7 +142,7 @@ allow(default_templatedir).to receive(:object_config).and_return(configs_hash) allow(cli_templatedir).to receive(:object_config).and_return(configs_hash) allow(metadata_templatedir).to receive(:object_config).and_return(configs_hash) - allow(PDK::Module::TemplateDir).to receive(:new).with(PDK::Util.default_template_uri).and_yield(default_templatedir) + allow(PDK::Module::TemplateDir).to receive(:new).with(any_args).and_yield(default_templatedir) end context 'when a template-url is provided on the CLI' do diff --git a/spec/unit/pdk/module/convert_spec.rb b/spec/unit/pdk/module/convert_spec.rb index 840017c49..72bab1f4f 100644 --- a/spec/unit/pdk/module/convert_spec.rb +++ b/spec/unit/pdk/module/convert_spec.rb @@ -344,19 +344,32 @@ end end - describe '#template_url' do - subject { described_class.new(options).template_url } + describe '#template_uri' do + subject { described_class.new(options).template_uri } let(:options) { {} } + before(:each) do + allow(PDK::Util).to receive(:package_install?).and_return(false) + allow(PDK::Util::Git).to receive(:repo?).and_call_original + end + context 'when a template-url is provided in the options' do let(:options) { { :'template-url' => 'https://my/custom/template' } } - it { is_expected.to eq('https://my/custom/template') } + before(:each) do + allow(PDK::Util::Git).to receive(:repo?).with(options[:'template-url']).and_return(true) + end + + it { is_expected.to eq(PDK::Util::TemplateURI.new('https://my/custom/template')) } end context 'when no template-url is provided in the options' do - it { is_expected.to eq(PDK::Util.default_template_url) } + before(:each) do + allow(PDK::Util::Git).to receive(:repo?).with(PDK::Util::TemplateURI.default_template_uri.metadata_format).and_return(true) + end + + it { is_expected.to eq(PDK::Util::TemplateURI.default_template_uri) } end end diff --git a/spec/unit/pdk/module/template_dir_spec.rb b/spec/unit/pdk/module/template_dir_spec.rb index 5284a858d..8ba6c2a2f 100644 --- a/spec/unit/pdk/module/template_dir_spec.rb +++ b/spec/unit/pdk/module/template_dir_spec.rb @@ -8,10 +8,9 @@ end end - let(:root_dir) { Gem.win_platform? ? 'C:/' : '/' } - let(:path_or_url) { File.join(root_dir, 'path', 'to', 'templates') } - let(:uri) { Gem.win_platform? ? Addressable::URI.parse('/' + path_or_url) : Addressable::URI.parse(path_or_url) } - let(:tmp_path) { File.join(root_dir, 'tmp', 'path') } + let(:path_or_url) { File.join('/', 'path', 'to', 'templates') } + let(:uri) { PDK::Util::TemplateURI.new(path_or_url) } + let(:tmp_path) { File.join('/', 'tmp', 'path') } let(:module_metadata) do { @@ -185,13 +184,14 @@ describe '.config_for(dest_path)' do before(:each) do + allow(Gem).to receive(:win_platform?).and_return(false) allow(File).to receive(:directory?).with(anything).and_return(true) allow(PDK::Util::Git).to receive(:repo?).with(path_or_url).and_return(false) allow(PDK::Util).to receive(:make_tmpdir_name).with('pdk-templates').and_return(tmp_path) allow(PDK::CLI::Exec).to receive(:git).with('clone', path_or_url, tmp_path).and_return(exit_code: 0) allow(File).to receive(:file?).with(anything).and_return(File.join(path_or_url, 'config_defaults.yml')).and_return(true) allow(File).to receive(:read).with(File.join(path_or_url, 'config_defaults.yml')).and_return(config_defaults) - allow(File).to receive(:readable?).with(File.join(path_or_url, 'config_defaults.yml')).and_return true + allow(File).to receive(:readable?).with(File.join(path_or_url, 'config_defaults.yml')).and_return(true) allow(YAML).to receive(:safe_load).with(config_defaults, [], [], true).and_return config_hash end @@ -221,10 +221,10 @@ end before(:each) do - allow(File).to receive(:file?).with('/path/to/module/.sync.yml').and_return true - allow(File).to receive(:readable?).with('/path/to/module/.sync.yml').and_return true - allow(File).to receive(:read).with('/path/to/module/.sync.yml').and_return yaml_text - allow(YAML).to receive(:safe_load).with(yaml_text, [], [], true).and_return yaml_hash + allow(File).to receive(:file?).with('/path/to/module/.sync.yml').and_return(true) + allow(File).to receive(:readable?).with('/path/to/module/.sync.yml').and_return(true) + allow(File).to receive(:read).with('/path/to/module/.sync.yml').and_return(yaml_text) + allow(YAML).to receive(:safe_load).with(yaml_text, [], [], true).and_return(yaml_hash) allow(PDK::Util).to receive(:module_root).and_return('/path/to/module') end @@ -326,7 +326,7 @@ allow(File).to receive(:directory?).with(anything).and_return(true) allow(PDK::Util::Git).to receive(:repo?).with(path_or_url).and_return(true) allow(PDK::Util).to receive(:default_template_url).and_return(path_or_url) - allow(PDK::Util).to receive(:default_template_ref).and_return('default-ref') + allow(PDK::Util::TemplateURI).to receive(:default_template_ref).and_return('default-ref') allow(PDK::Util).to receive(:make_tmpdir_name).with('pdk-templates').and_return(tmp_path) allow(Dir).to receive(:chdir).with(tmp_path).and_yield allow(PDK::Util::Git).to receive(:git).with('clone', path_or_url, tmp_path).and_return(exit_code: 0) @@ -339,6 +339,7 @@ "default-sha\trefs/remotes/origin/default-ref") allow(PDK::Util::Version).to receive(:version_string).and_return('0.0.0') allow(PDK::Util).to receive(:canonical_path).with(tmp_path).and_return(tmp_path) + allow(PDK::Util).to receive(:development_mode?).and_return(false) end context 'pdk data' do @@ -353,7 +354,7 @@ allow(File).to receive(:directory?).with(anything).and_return(true) allow(PDK::Util::Git).to receive(:repo?).with(path_or_url).and_return(true) allow(PDK::Util).to receive(:default_template_url).and_return('default-url') - allow(PDK::Util).to receive(:default_template_ref).and_return('default-ref') + allow(PDK::Util::TemplateURI).to receive(:default_template_ref).and_return('default-ref') allow(PDK::Util).to receive(:make_tmpdir_name).with('pdk-templates').and_return(tmp_path) allow(Dir).to receive(:chdir).with(tmp_path).and_yield allow(PDK::Util::Git).to receive(:git).with('clone', path_or_url, tmp_path).and_return(exit_code: 0) @@ -366,6 +367,7 @@ "default-sha\trefs/remotes/origin/default-ref") allow(PDK::Util::Version).to receive(:version_string).and_return('0.0.0') allow(PDK::Util).to receive(:canonical_path).with(tmp_path).and_return(tmp_path) + allow(PDK::Util).to receive(:development_mode?).and_return(false) end context 'pdk data' do diff --git a/spec/unit/pdk/module/update_spec.rb b/spec/unit/pdk/module/update_spec.rb index cc85dc665..a9f9049ee 100644 --- a/spec/unit/pdk/module/update_spec.rb +++ b/spec/unit/pdk/module/update_spec.rb @@ -99,7 +99,7 @@ context 'when using the default template' do let(:options) { { noop: true } } - let(:template_url) { PDK::Util.default_template_uri } + let(:template_url) { PDK::Util::TemplateURI.default_template_uri } it 'refers to the template as the default template' do expect(logger).to receive(:info).with(a_string_matching(%r{using the default template}i)) @@ -241,7 +241,7 @@ context 'when the default_template_ref specifies a tag' do before(:each) do - allow(PDK::Util).to receive(:default_template_ref).and_return(PDK::TEMPLATE_REF) + allow(PDK::Util).to receive(:development_mode?).and_return(false) end it 'returns the tag name' do diff --git a/spec/unit/pdk/util/template_uri_spec.rb b/spec/unit/pdk/util/template_uri_spec.rb index 5b3213dd6..d1821c38f 100644 --- a/spec/unit/pdk/util/template_uri_spec.rb +++ b/spec/unit/pdk/util/template_uri_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' describe PDK::Util::TemplateURI do - before :all do + before(:each) do PDK.answers.update!('template-url' => nil) end @@ -79,8 +79,10 @@ context 'and the answer file template is invalid' do before(:each) do + # rubocop:disable RSpec/SubjectStub allow(template_uri).to receive(:valid_template?).with(anything).and_call_original allow(template_uri).to receive(:valid_template?).with(uri: anything, type: anything, allow_fallback: true).and_return(false) + # rubocop:enable RSpec/SubjectStub end it 'returns the default template' do @@ -301,5 +303,4 @@ end end end - end From eaf9ece8759572f705aae5812ef590f940cbefc1 Mon Sep 17 00:00:00 2001 From: Tim Sharpe Date: Mon, 11 Feb 2019 13:39:53 +1100 Subject: [PATCH 10/24] WIP Tests --- lib/pdk/util/template_uri.rb | 7 +- spec/acceptance/new_module_spec.rb | 20 +++ spec/unit/pdk/module/template_dir_spec.rb | 159 ++++++++++++++++++++++ spec/unit/pdk/util/template_uri_spec.rb | 102 ++++++++++++-- 4 files changed, 278 insertions(+), 10 deletions(-) diff --git a/lib/pdk/util/template_uri.rb b/lib/pdk/util/template_uri.rb index 0c24d4545..92a7e266f 100644 --- a/lib/pdk/util/template_uri.rb +++ b/lib/pdk/util/template_uri.rb @@ -208,7 +208,12 @@ def valid_template?(template) PDK::Module::TemplateDir.new(template[:uri]) {} return true end - return true if PDK::Util::Git.repo?(template[:uri].to_s) + repo = if template[:uri].fragment + template[:uri].to_s.chomp("##{template[:uri].fragment}") + else + template[:uri].to_s + end + return true if PDK::Util::Git.repo?(repo) false rescue ArgumentError diff --git a/spec/acceptance/new_module_spec.rb b/spec/acceptance/new_module_spec.rb index 87b6c83e9..fb078577e 100644 --- a/spec/acceptance/new_module_spec.rb +++ b/spec/acceptance/new_module_spec.rb @@ -47,4 +47,24 @@ end end end + + context 'when --template-url and --template-ref are used' do + after(:all) do + FileUtils.rm_rf('foo_ref') + end + + describe command('pdk new module foo_ref --skip-interview --template-url https://github.com/puppetlabs/pdk-templates --template-ref 1.7.0') do + its(:exit_status) { is_expected.to eq(0) } + its(:stderr) { is_expected.to match(%r{creating new module: foo_ref}i) } + its(:stderr) { is_expected.not_to match(%r{WARN|ERR}) } + its(:stdout) { is_expected.to match(%r{\A\Z}) } + end + + describe file('foo_ref/metadata.json') do + it { is_expected.to be_file } + its(:content_as_json) do + is_expected.to include('template-ref' => match(%r{1\.7\.0})) + end + end + end end diff --git a/spec/unit/pdk/module/template_dir_spec.rb b/spec/unit/pdk/module/template_dir_spec.rb index 8ba6c2a2f..bbfa13371 100644 --- a/spec/unit/pdk/module/template_dir_spec.rb +++ b/spec/unit/pdk/module/template_dir_spec.rb @@ -30,6 +30,165 @@ EOS end + describe '.new' do + context 'when not passed a block' do + it 'raises an ArgumentError' do + expect { + described_class.new(uri, module_metadata) + }.to raise_error(ArgumentError, %r{must be initialized with a block}i) + end + end + + context 'when not initialized with a PDK::Util::TemplateURI' do + it 'raises an ArgumentError' do + expect { + described_class.new(path_or_url, module_metadata) {} + }.to raise_error(ArgumentError, %r{must be initialized with a PDK::Util::TemplateURI}i) + end + end + end + + describe '#validate_module_template!' do + let(:moduleroot) { File.join(path_or_url, 'moduleroot') } + let(:moduleroot_init) { File.join(path_or_url, 'moduleroot_init') } + + before(:each) do + allow(File).to receive(:directory?).with(anything).and_return(true) + end + + context 'when the template path is a directory' do + before(:each) do + allow(File).to receive(:directory?).with(path_or_url).and_return(true) + end + + context 'and the template contains a moduleroot directory' do + before(:each) do + allow(File).to receive(:directory?).with(moduleroot).and_return(true) + end + + context 'and a moduleroot_init directory' do + before(:each) do + allow(File).to receive(:directory?).with(moduleroot_init).and_return(true) + end + + it 'does not raise an error' do + expect { described_class.new(uri, module_metadata) {} }.not_to raise_error + end + end + + context 'but not a moduleroot_init directory' do + before(:each) do + allow(File).to receive(:directory?).with(moduleroot_init).and_return(false) + end + + it 'raises an ArgumentError' do + expect { + described_class.new(uri, module_metadata) {} + }.to raise_error(ArgumentError, %r{does not contain a 'moduleroot_init/'}) + end + end + end + + context 'and the template does not contain a moduleroot directory' do + before(:each) do + allow(File).to receive(:directory?).with(moduleroot).and_return(false) + end + + it 'raises an ArgumentError' do + expect { + described_class.new(uri, module_metadata) {} + }.to raise_error(ArgumentError, %r{does not contain a 'moduleroot/'}) + end + end + end + + context 'when the template path is not a directory' do + before(:each) do + allow(File).to receive(:directory?).with(path_or_url).and_return(false) + allow(PDK::Util).to receive(:package_install?).and_return(false) + end + + context 'and it specifies an deprecated built-in template' do + before(:each) do + allow(PDK::Util).to receive(:package_install?).and_return(true) + allow(File).to receive(:fnmatch?).with(anything, path_or_url).and_return(true) + allow(PDK::Util).to receive(:package_cachedir).and_return(File.join('/', 'path', 'to', 'package', 'cachedir')) + allow(described_class).to receive(:clone_template_repo).and_return(path_or_url) + allow(PDK::Util::Git).to receive(:repo?).with(path_or_url).and_return(true) + allow(FileUtils).to receive(:remove_dir) + end + + it 'raises an ArgumentError' do + expect { + described_class.new(uri, module_metadata) {} + }.to raise_error(ArgumentError, %r{built-in template has substantially changed}) + end + end + + it 'raises an ArgumentError' do + expect { + described_class.new(uri, module_metadata) {} + }.to raise_error(ArgumentError, %r{is not a directory}) + end + end + end + + describe '.checkout_template_ref' do + let(:path) { File.join('/', 'path', 'to', 'workdir') } + let(:ref) { '12345678' } + let(:full_ref) { '123456789abcdef' } + + context 'when the template workdir is clean' do + before(:each) do + allow(PDK::Util::Git).to receive(:work_dir_clean?).with(path).and_return(true) + allow(Dir).to receive(:chdir).with(path).and_yield + allow(PDK::Util::Git).to receive(:ls_remote).with(path, ref).and_return(full_ref) + end + + context 'and the git reset succeeds' do + before(:each) do + allow(PDK::Util::Git).to receive(:git).with('reset', '--hard', full_ref).and_return(exit_code: 0) + end + + it 'does not raise an error' do + expect { + described_class.checkout_template_ref(path, ref) + }.not_to raise_error + end + end + + context 'and the git reset fails' do + let(:result) { { exit_code: 1, stderr: 'stderr', stdout: 'stdout' } } + + before(:each) do + allow(PDK::Util::Git).to receive(:git).with('reset', '--hard', full_ref).and_return(result) + end + + it 'raises a FatalError' do + expect(logger).to receive(:error).with(result[:stdout]) + expect(logger).to receive(:error).with(result[:stderr]) + expect { + described_class.checkout_template_ref(path, ref) + }.to raise_error(PDK::CLI::FatalError, %r{unable to set head of git repository}i) + end + end + end + + context 'when the template workdir is not clean' do + before(:each) do + allow(PDK::Util::Git).to receive(:work_dir_clean?).with(path).and_return(false) + end + + after(:each) do + described_class.checkout_template_ref(path, ref) + end + + it 'warns the user' do + expect(logger).to receive(:warn).with(a_string_matching(%r{uncommitted changes found}i)) + end + end + end + context 'with a valid template path' do it 'returns config hash with module metadata' do allow(File).to receive(:directory?).with(anything).and_return(true) diff --git a/spec/unit/pdk/util/template_uri_spec.rb b/spec/unit/pdk/util/template_uri_spec.rb index d1821c38f..66a174f0a 100644 --- a/spec/unit/pdk/util/template_uri_spec.rb +++ b/spec/unit/pdk/util/template_uri_spec.rb @@ -11,19 +11,33 @@ describe '.new' do context 'with a string' do - let(:opts_or_uri) { 'https://github.com/my/pdk-templates.git#custom' } + context 'that contains a valid URI' do + let(:opts_or_uri) { 'https://github.com/my/pdk-templates.git#custom' } - it 'can return a string for storing' do - expect(template_uri.to_s).to eq('https://github.com/my/pdk-templates.git#custom') + it 'can return a string for storing' do + expect(template_uri.to_s).to eq('https://github.com/my/pdk-templates.git#custom') + end + end + + context 'that contains an invalid URI' do + let(:opts_or_uri) { 'https://' } + + it 'raises a FatalError' do + expect { + template_uri + }.to raise_error(PDK::CLI::FatalError, %r{initialization with a non-uri string}i) + end end end - context 'with an addressable::uri' do + + context 'with an Addressable::URI' do let(:opts_or_uri) { Addressable::URI.parse('https://github.com/my/pdk-templates.git#custom') } it 'can return a string for storing' do expect(template_uri.to_s).to eq('https://github.com/my/pdk-templates.git#custom') end end + context 'with options' do let(:opts_or_uri) do { @@ -37,6 +51,7 @@ expect(template_uri.to_s).to eq('https://github.com/my/pdk-templates.git#custom') end end + context 'combinations of answers, options, and defaults' do before :each do allow(PDK::Util::Git).to receive(:repo?).with(anything).and_return(true) @@ -79,14 +94,13 @@ context 'and the answer file template is invalid' do before(:each) do - # rubocop:disable RSpec/SubjectStub - allow(template_uri).to receive(:valid_template?).with(anything).and_call_original - allow(template_uri).to receive(:valid_template?).with(uri: anything, type: anything, allow_fallback: true).and_return(false) - # rubocop:enable RSpec/SubjectStub + # rubocop:disable RSpec/AnyInstance + allow_any_instance_of(described_class).to receive(:valid_template?).with(anything).and_call_original + allow_any_instance_of(described_class).to receive(:valid_template?).with(uri: anything, type: anything, allow_fallback: true).and_return(false) + # rubocop:enable RSpec/AnyInstance end it 'returns the default template' do - pending expect(template_uri.to_s).to eq(described_class.default_template_uri.to_s) end end @@ -303,4 +317,74 @@ end end end + + describe '.templates' do + subject { described_class.templates(options) } + + let(:options) { {} } + + context 'when provided a ref but not a url' do + let(:options) { { :'template-ref' => '123' } } + + it 'raises a FatalError' do + expect { + described_class.templates(options) + }.to raise_error(PDK::CLI::FatalError, %r{--template-ref requires --template-url}) + end + end + + context 'when provided a url that contains a hash' do + let(:options) { { :'template-url' => 'https://github.com/puppetlabs/pdk-templates#master' } } + + it 'raises a FatalError' do + expect { + described_class.templates(options) + }.to raise_error(PDK::CLI::FatalError, %r{may not be used to specify paths containing #}i) + end + end + + context 'when the answers file has saved template-url value' do + before(:each) do + PDK.answers.update!('template-url' => answers_template_url) + end + + after(:each) do + PDK.answers.update!('template-url' => nil) + end + + context 'that is the deprecated pdk-module-template' do + let(:answers_template_url) { 'https://github.com/puppetlabs/pdk-module-template' } + + it 'converts it to the new default template URL' do + is_expected.to include( + type: 'PDK answers', + uri: Addressable::URI.parse('https://github.com/puppetlabs/pdk-templates'), + allow_fallback: true, + ) + end + end + + context 'that contains any other URL' do + let(:answers_template_url) { 'https://github.com/my/pdk-template' } + + it 'uses the template as specified' do + is_expected.to include( + type: 'PDK answers', + uri: Addressable::URI.parse(answers_template_url), + allow_fallback: true, + ) + end + end + end + + context 'when the answers file has no saved template-url value' do + before(:each) do + PDK.answers.update!('template-url' => nil) + end + + xit 'does not include a PDK answers template option' do + is_expected.not_to include(type: 'PDK answers', uri: anything, allow_fallback: true) + end + end + end end From fd0b9e409234a584110f93cf99dc54a483b16d0b Mon Sep 17 00:00:00 2001 From: Tim Sharpe Date: Fri, 15 Feb 2019 15:26:15 +1100 Subject: [PATCH 11/24] Don't include answers uri if nil --- lib/pdk/util/template_uri.rb | 2 +- spec/unit/pdk/util/template_uri_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/pdk/util/template_uri.rb b/lib/pdk/util/template_uri.rb index 92a7e266f..eeeb96ce4 100644 --- a/lib/pdk/util/template_uri.rb +++ b/lib/pdk/util/template_uri.rb @@ -172,7 +172,7 @@ def self.templates(opts) ary = [] ary << { type: _('--template-url'), uri: explicit_uri, allow_fallback: false } ary << { type: _('metadata.json'), uri: metadata_uri, allow_fallback: true } if metadata_uri - ary << { type: _('PDK answers'), uri: answers_uri, allow_fallback: true } unless metadata_uri + ary << { type: _('PDK answers'), uri: answers_uri, allow_fallback: true } if answers_uri ary << { type: _('default'), uri: default_uri, allow_fallback: false } ary end diff --git a/spec/unit/pdk/util/template_uri_spec.rb b/spec/unit/pdk/util/template_uri_spec.rb index 66a174f0a..9f75aacdc 100644 --- a/spec/unit/pdk/util/template_uri_spec.rb +++ b/spec/unit/pdk/util/template_uri_spec.rb @@ -382,7 +382,7 @@ PDK.answers.update!('template-url' => nil) end - xit 'does not include a PDK answers template option' do + it 'does not include a PDK answers template option' do is_expected.not_to include(type: 'PDK answers', uri: anything, allow_fallback: true) end end From 860f6273d6283d1c4840b115870b062b22b30442 Mon Sep 17 00:00:00 2001 From: Tim Sharpe Date: Fri, 15 Feb 2019 17:03:55 +1100 Subject: [PATCH 12/24] Honor --template-ref in pdk update --- lib/pdk/cli/update.rb | 2 ++ lib/pdk/module/update.rb | 19 +++++------ lib/pdk/util/template_uri.rb | 4 +++ spec/acceptance/new_module_spec.rb | 20 ------------ spec/acceptance/template_ref_spec.rb | 48 ++++++++++++++++++++++++++++ spec/unit/pdk/module/update_spec.rb | 4 +-- 6 files changed, 64 insertions(+), 33 deletions(-) create mode 100644 spec/acceptance/template_ref_spec.rb diff --git a/lib/pdk/cli/update.rb b/lib/pdk/cli/update.rb index e92cae5ae..e7565c57f 100644 --- a/lib/pdk/cli/update.rb +++ b/lib/pdk/cli/update.rb @@ -10,6 +10,8 @@ module PDK::CLI flag nil, :noop, _('Do not update the module, just output what would be done.') flag nil, :force, _('Update the module automatically, with no prompts.') + PDK::CLI.template_ref_option(self) + run do |opts, _args, _cmd| require 'pdk/module/update' diff --git a/lib/pdk/module/update.rb b/lib/pdk/module/update.rb index 12a9a3171..1ea6a739d 100644 --- a/lib/pdk/module/update.rb +++ b/lib/pdk/module/update.rb @@ -6,6 +6,8 @@ class Update < Convert GIT_DESCRIBE_PATTERN = %r{\A(?.+?)-(?\d+)-g(?.+)\Z} def run + template_uri.git_ref = new_template_version + stage_changes! if current_version == new_version @@ -50,11 +52,8 @@ def module_metadata raise PDK::CLI::ExitWithError, e.message end - # TODO: update should be able to use exsting url. uri fragments should be - # tracked if existing, but the template-ref in the metadata is not for - # tracking simply for referencing the last update target. - def metadata_template_uri - @metadata_template_uri ||= PDK::Util::TemplateURI.new(module_metadata.data['template-url']) + def template_uri + @template_uri ||= PDK::Util::TemplateURI.new(module_metadata.data['template-url']) end def current_version @@ -68,8 +67,6 @@ def new_version private def current_template_version - # TODO: Should the current/new versions be queried here or should that - # come from the util class? @current_template_version ||= module_metadata.data['template-ref'] end @@ -86,7 +83,7 @@ def describe_ref_to_s(describe_ref) end def new_template_version - metadata_template_uri.git_ref + options.fetch(:'template-ref', PDK::Util::TemplateURI.default_template_ref) end def fetch_remote_version(template_ref) @@ -94,11 +91,11 @@ def fetch_remote_version(template_ref) return template_ref if template_ref == PDK::TEMPLATE_REF sha_length = GIT_DESCRIBE_PATTERN.match(current_template_version)[:sha].length - 1 - "#{template_ref}@#{PDK::Util::Git.ls_remote(metadata_template_uri.git_remote, template_ref)[0..sha_length]}" + "#{template_ref}@#{PDK::Util::Git.ls_remote(template_uri.git_remote, template_ref)[0..sha_length]}" end def update_message - format_string = if metadata_template_uri == PDK::Util::TemplateURI.default_template_uri + format_string = if template_uri.git_remote == PDK::Util::TemplateURI.default_template_uri.git_remote _('Updating %{module_name} using the default template, from %{current_version} to %{new_version}') else _('Updating %{module_name} using the template at %{template_url}, from %{current_version} to %{new_version}') @@ -106,7 +103,7 @@ def update_message format_string % { module_name: module_metadata.data['name'], - template_url: metadata_template_uri.git_remote, + template_url: template_uri.git_remote, current_version: current_version, new_version: new_version, } diff --git a/lib/pdk/util/template_uri.rb b/lib/pdk/util/template_uri.rb index eeeb96ce4..144380ba6 100644 --- a/lib/pdk/util/template_uri.rb +++ b/lib/pdk/util/template_uri.rb @@ -77,6 +77,10 @@ def git_ref end end + def git_ref=(ref) + @uri.fragment = ref + end + # @returns PDK::Util::TemplateURI def self.default_template_uri if PDK::Util.package_install? diff --git a/spec/acceptance/new_module_spec.rb b/spec/acceptance/new_module_spec.rb index fb078577e..87b6c83e9 100644 --- a/spec/acceptance/new_module_spec.rb +++ b/spec/acceptance/new_module_spec.rb @@ -47,24 +47,4 @@ end end end - - context 'when --template-url and --template-ref are used' do - after(:all) do - FileUtils.rm_rf('foo_ref') - end - - describe command('pdk new module foo_ref --skip-interview --template-url https://github.com/puppetlabs/pdk-templates --template-ref 1.7.0') do - its(:exit_status) { is_expected.to eq(0) } - its(:stderr) { is_expected.to match(%r{creating new module: foo_ref}i) } - its(:stderr) { is_expected.not_to match(%r{WARN|ERR}) } - its(:stdout) { is_expected.to match(%r{\A\Z}) } - end - - describe file('foo_ref/metadata.json') do - it { is_expected.to be_file } - its(:content_as_json) do - is_expected.to include('template-ref' => match(%r{1\.7\.0})) - end - end - end end diff --git a/spec/acceptance/template_ref_spec.rb b/spec/acceptance/template_ref_spec.rb new file mode 100644 index 000000000..6afeca472 --- /dev/null +++ b/spec/acceptance/template_ref_spec.rb @@ -0,0 +1,48 @@ +require 'spec_helper_acceptance' +require 'pdk/version' + +describe 'Specifying a template-ref' do + after(:all) do + FileUtils.rm_rf('foo') + FileUtils.rm('foo_answers.json') + end + + context 'when creating a new module' do + create_cmd = [ + 'pdk', 'new', 'module', 'foo', + '--skip-interview', + '--template-url', 'https://github.com/puppetlabs/pdk-templates', + '--template-ref', '1.7.0', + '--answer-file', File.join(Dir.pwd, "foo_answers.json"), + ] + + describe command(create_cmd.join(' ')) do + its(:exit_status) { is_expected.to eq(0) } + its(:stderr) { is_expected.to match(%r{creating new module: foo}i) } + its(:stderr) { is_expected.not_to match(%r{WARN|ERR}) } + its(:stdout) { is_expected.to match(%r{\A\Z}) } + + describe file('foo/metadata.json') do + it { is_expected.to be_file } + its(:content_as_json) do + is_expected.to include('template-ref' => match(%r{1\.7\.0})) + end + end + end + + context 'and then updating the module to a specific ref' do + before(:all) { Dir.chdir('foo') } + after(:all) { Dir.chdir('..') } + + describe command('pdk update --template-ref 1.8.0 --force') do + its(:exit_status) { is_expected.to eq(0) } + + describe file('metadata.json') do + its(:content_as_json) do + is_expected.to include('template-ref' => match(%r{1\.8\.0})) + end + end + end + end + end +end diff --git a/spec/unit/pdk/module/update_spec.rb b/spec/unit/pdk/module/update_spec.rb index a9f9049ee..96b66ad43 100644 --- a/spec/unit/pdk/module/update_spec.rb +++ b/spec/unit/pdk/module/update_spec.rb @@ -202,8 +202,8 @@ end end - describe '#metadata_template_uri' do - subject { described_class.new(options).metadata_template_uri.to_s } + describe '#template_uri' do + subject { described_class.new(options).template_uri.to_s } include_context 'with mock metadata' From c39ab4989a0d46527807285f8ccc0be0e3100c88 Mon Sep 17 00:00:00 2001 From: Tim Sharpe Date: Fri, 15 Feb 2019 17:04:36 +1100 Subject: [PATCH 13/24] Respect allow_fallback when validating template choices --- lib/pdk/util/template_uri.rb | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/lib/pdk/util/template_uri.rb b/lib/pdk/util/template_uri.rb index 144380ba6..a9ed1fe97 100644 --- a/lib/pdk/util/template_uri.rb +++ b/lib/pdk/util/template_uri.rb @@ -219,6 +219,12 @@ def valid_template?(template) end return true if PDK::Util::Git.repo?(repo) + unless template[:allow_fallback] + raise PDK::CLI::FatalError, _('Unable to find a valid template at %{uri}') % { + uri: template[:uri].to_s, + } + end + false rescue ArgumentError return false From 9dcd46ad3a11785c7772ee35701d9782d1685f0b Mon Sep 17 00:00:00 2001 From: Tim Sharpe Date: Mon, 18 Feb 2019 09:15:25 +1100 Subject: [PATCH 14/24] fix rubocop errors --- spec/acceptance/template_ref_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/acceptance/template_ref_spec.rb b/spec/acceptance/template_ref_spec.rb index 6afeca472..bf9cf781f 100644 --- a/spec/acceptance/template_ref_spec.rb +++ b/spec/acceptance/template_ref_spec.rb @@ -4,7 +4,7 @@ describe 'Specifying a template-ref' do after(:all) do FileUtils.rm_rf('foo') - FileUtils.rm('foo_answers.json') + FileUtils.rm_f('foo_answers.json') end context 'when creating a new module' do @@ -13,7 +13,7 @@ '--skip-interview', '--template-url', 'https://github.com/puppetlabs/pdk-templates', '--template-ref', '1.7.0', - '--answer-file', File.join(Dir.pwd, "foo_answers.json"), + '--answer-file', File.join(Dir.pwd, 'foo_answers.json') ] describe command(create_cmd.join(' ')) do From 8fb4799b464e3986d25e36e7ff5ca654b1ac2117 Mon Sep 17 00:00:00 2001 From: Tim Sharpe Date: Mon, 18 Feb 2019 16:30:44 +1100 Subject: [PATCH 15/24] Consistently include template-ref --- lib/pdk/generate/module.rb | 2 +- lib/pdk/util/template_uri.rb | 3 ++- spec/unit/pdk/generate/module_spec.rb | 9 +++++---- spec/unit/pdk/generate/puppet_object_spec.rb | 3 ++- spec/unit/pdk/module/convert_spec.rb | 5 +++-- spec/unit/pdk/util/template_uri_spec.rb | 9 +++++---- 6 files changed, 18 insertions(+), 13 deletions(-) diff --git a/lib/pdk/generate/module.rb b/lib/pdk/generate/module.rb index f08165898..8b20d3a5e 100644 --- a/lib/pdk/generate/module.rb +++ b/lib/pdk/generate/module.rb @@ -75,7 +75,7 @@ def self.invoke(opts = {}) end # Only update the answers files after metadata has been written. - if template_uri.metadata_format == PDK::Util::TemplateURI.default_template_uri.metadata_format + if template_uri.git_remote == PDK::Util::TemplateURI.default_template_uri.git_remote # If the user specifies our default template url via the command # line, remove the saved template-url answer so that the template_uri # resolution can find new default URLs in the future. diff --git a/lib/pdk/util/template_uri.rb b/lib/pdk/util/template_uri.rb index a9ed1fe97..03806e37c 100644 --- a/lib/pdk/util/template_uri.rb +++ b/lib/pdk/util/template_uri.rb @@ -154,7 +154,7 @@ def self.templates(opts) end end explicit_uri = Addressable::URI.parse(uri_safe(explicit_url)) - explicit_uri.fragment = explicit_ref + explicit_uri.fragment = explicit_ref || default_template_ref else explicit_uri = nil end @@ -172,6 +172,7 @@ def self.templates(opts) nil end default_uri = Addressable::URI.parse(default_template_uri) + default_uri.fragment = default_template_ref ary = [] ary << { type: _('--template-url'), uri: explicit_uri, allow_fallback: false } diff --git a/spec/unit/pdk/generate/module_spec.rb b/spec/unit/pdk/generate/module_spec.rb index 30357d03f..ed3463f16 100644 --- a/spec/unit/pdk/generate/module_spec.rb +++ b/spec/unit/pdk/generate/module_spec.rb @@ -184,7 +184,7 @@ end it 'uses that template to generate the module' do - expect(PDK::Module::TemplateDir).to receive(:new).with(Addressable::URI.parse('cli-template'), anything, anything).and_yield(test_template_dir) + expect(PDK::Module::TemplateDir).to receive(:new).with(Addressable::URI.parse('cli-template#master'), anything, anything).and_yield(test_template_dir) expect(logger).to receive(:info).with(a_string_matching(%r{generated at path}i)) expect(logger).to receive(:info).with(a_string_matching(%r{In your module directory, add classes with the 'pdk new class' command}i)) @@ -193,12 +193,12 @@ it 'takes precedence over the template-url answer' do PDK.answers.update!('template-url' => 'answer-template') - expect(PDK::Module::TemplateDir).to receive(:new).with(Addressable::URI.parse('cli-template'), anything, anything).and_yield(test_template_dir) + expect(PDK::Module::TemplateDir).to receive(:new).with(Addressable::URI.parse('cli-template#master'), anything, anything).and_yield(test_template_dir) described_class.invoke(invoke_opts.merge(:'template-url' => 'cli-template')) end it 'saves the template-url and template-ref to the answer file if it is not the default template' do - expect(PDK.answers).to receive(:update!).with('template-url' => Addressable::URI.parse('cli-template')) + expect(PDK.answers).to receive(:update!).with('template-url' => Addressable::URI.parse('cli-template#master')) described_class.invoke(invoke_opts.merge(:'template-url' => 'cli-template')) end @@ -236,7 +236,8 @@ end it 'uses the vendored template url' do - expect(PDK::Module::TemplateDir).to receive(:new).with(Addressable::URI.parse('file:///tmp/package/cache/pdk-templates.git'), anything, anything).and_yield(test_template_dir) + template_uri = "file:///tmp/package/cache/pdk-templates.git##{PDK::Util::TemplateURI.default_template_ref}" + expect(PDK::Module::TemplateDir).to receive(:new).with(Addressable::URI.parse(template_uri), anything, anything).and_yield(test_template_dir) expect(PDK.answers).not_to receive(:update!).with(:'template-url' => anything) described_class.invoke(invoke_opts) diff --git a/spec/unit/pdk/generate/puppet_object_spec.rb b/spec/unit/pdk/generate/puppet_object_spec.rb index be44f2668..206aeb1f6 100644 --- a/spec/unit/pdk/generate/puppet_object_spec.rb +++ b/spec/unit/pdk/generate/puppet_object_spec.rb @@ -143,13 +143,14 @@ allow(cli_templatedir).to receive(:object_config).and_return(configs_hash) allow(metadata_templatedir).to receive(:object_config).and_return(configs_hash) allow(PDK::Module::TemplateDir).to receive(:new).with(any_args).and_yield(default_templatedir) + allow(PDK::Util).to receive(:development_mode?).and_return(true) end context 'when a template-url is provided on the CLI' do let(:options) { { :'template-url' => '/some/path' } } before(:each) do - allow(PDK::Module::TemplateDir).to receive(:new).with(Addressable::URI.parse('/some/path')).and_yield(cli_templatedir) + allow(PDK::Module::TemplateDir).to receive(:new).with(Addressable::URI.parse('/some/path#master')).and_yield(cli_templatedir) end context 'and a template for the object type exists' do diff --git a/spec/unit/pdk/module/convert_spec.rb b/spec/unit/pdk/module/convert_spec.rb index 72bab1f4f..924c7165b 100644 --- a/spec/unit/pdk/module/convert_spec.rb +++ b/spec/unit/pdk/module/convert_spec.rb @@ -361,15 +361,16 @@ allow(PDK::Util::Git).to receive(:repo?).with(options[:'template-url']).and_return(true) end - it { is_expected.to eq(PDK::Util::TemplateURI.new('https://my/custom/template')) } + it { is_expected.to eq(PDK::Util::TemplateURI.new("https://my/custom/template##{PDK::Util::TemplateURI.default_template_ref}")) } end context 'when no template-url is provided in the options' do before(:each) do allow(PDK::Util::Git).to receive(:repo?).with(PDK::Util::TemplateURI.default_template_uri.metadata_format).and_return(true) end + let(:default_uri) { "#{PDK::Util::TemplateURI.default_template_uri}##{PDK::Util::TemplateURI.default_template_ref}" } - it { is_expected.to eq(PDK::Util::TemplateURI.default_template_uri) } + it { is_expected.to eq(PDK::Util::TemplateURI.new(default_uri)) } end end diff --git a/spec/unit/pdk/util/template_uri_spec.rb b/spec/unit/pdk/util/template_uri_spec.rb index 9f75aacdc..fb9a05dd8 100644 --- a/spec/unit/pdk/util/template_uri_spec.rb +++ b/spec/unit/pdk/util/template_uri_spec.rb @@ -74,6 +74,7 @@ end let(:opts_or_uri) { {} } + let(:default_uri) { "#{described_class.default_template_uri}##{described_class.default_template_ref}" } context 'when passed no options' do context 'and there are no metadata or answers' do @@ -81,7 +82,7 @@ PDK.answers.update!('template-url' => nil) end it 'returns the default template' do - expect(template_uri.to_s).to eq(described_class.default_template_uri.to_s) + expect(template_uri.to_s).to eq(default_uri) end end context 'and there are only answers' do @@ -101,7 +102,7 @@ end it 'returns the default template' do - expect(template_uri.to_s).to eq(described_class.default_template_uri.to_s) + expect(template_uri.to_s).to eq(default_uri) end end end @@ -127,7 +128,7 @@ let(:opts_or_uri) { { :'template-url' => 'cli-templates' } } it 'returns the specified template' do - expect(template_uri.to_s).to eq('cli-templates') + expect(template_uri.to_s).to eq("cli-templates##{described_class.default_template_ref}") end end context 'and passed windows template-url' do @@ -135,7 +136,7 @@ it 'returns the specified template' do allow(Gem).to receive(:win_platform?).and_return(true) - expect(template_uri.to_s).to eq('C:\cli-templates') + expect(template_uri.to_s).to eq("C:\\cli-templates##{described_class.default_template_ref}") end end context 'and passed template-ref' do From ca759389b62c93b9bd65a94d6ba6b37c1d6aa08c Mon Sep 17 00:00:00 2001 From: Tim Sharpe Date: Mon, 18 Feb 2019 16:39:34 +1100 Subject: [PATCH 16/24] clean up after template-ref acceptance tests --- spec/acceptance/template_ref_spec.rb | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/spec/acceptance/template_ref_spec.rb b/spec/acceptance/template_ref_spec.rb index bf9cf781f..8314870d5 100644 --- a/spec/acceptance/template_ref_spec.rb +++ b/spec/acceptance/template_ref_spec.rb @@ -1,10 +1,10 @@ require 'spec_helper_acceptance' -require 'pdk/version' describe 'Specifying a template-ref' do after(:all) do + Dir.chdir('..') FileUtils.rm_rf('foo') - FileUtils.rm_f('foo_answers.json') + FileUtils.rm('foo_answers.json') end context 'when creating a new module' do @@ -13,7 +13,7 @@ '--skip-interview', '--template-url', 'https://github.com/puppetlabs/pdk-templates', '--template-ref', '1.7.0', - '--answer-file', File.join(Dir.pwd, 'foo_answers.json') + '--answer-file', 'foo_answers.json' ] describe command(create_cmd.join(' ')) do @@ -32,7 +32,6 @@ context 'and then updating the module to a specific ref' do before(:all) { Dir.chdir('foo') } - after(:all) { Dir.chdir('..') } describe command('pdk update --template-ref 1.8.0 --force') do its(:exit_status) { is_expected.to eq(0) } From 9c5aad33cc8dbb805238fab042e7293da520128f Mon Sep 17 00:00:00 2001 From: Tim Sharpe Date: Mon, 18 Feb 2019 18:39:43 +1100 Subject: [PATCH 17/24] Validate local templates --- lib/pdk/util/template_uri.rb | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/lib/pdk/util/template_uri.rb b/lib/pdk/util/template_uri.rb index 03806e37c..c7fcfbfbc 100644 --- a/lib/pdk/util/template_uri.rb +++ b/lib/pdk/util/template_uri.rb @@ -209,10 +209,6 @@ def valid_template?(template) return false if template.nil? return false if template[:uri].nil? - if template[:path] && File.directory?(template[:path]) - PDK::Module::TemplateDir.new(template[:uri]) {} - return true - end repo = if template[:uri].fragment template[:uri].to_s.chomp("##{template[:uri].fragment}") else @@ -220,6 +216,12 @@ def valid_template?(template) end return true if PDK::Util::Git.repo?(repo) + path = self.class.human_readable(template[:uri].path) + if File.directory?(path) + PDK::Module::TemplateDir.new(path) {} + return true + end + unless template[:allow_fallback] raise PDK::CLI::FatalError, _('Unable to find a valid template at %{uri}') % { uri: template[:uri].to_s, From 9081643ca31024f24064f4b54edec77f0444b09c Mon Sep 17 00:00:00 2001 From: Tim Sharpe Date: Tue, 19 Feb 2019 14:18:48 +1100 Subject: [PATCH 18/24] Fill out valid_template? coverage --- lib/pdk/util/template_uri.rb | 41 ++++++------ spec/unit/pdk/util/template_uri_spec.rb | 86 +++++++++++++++++++++++-- 2 files changed, 102 insertions(+), 25 deletions(-) diff --git a/lib/pdk/util/template_uri.rb b/lib/pdk/util/template_uri.rb index c7fcfbfbc..dc5cac752 100644 --- a/lib/pdk/util/template_uri.rb +++ b/lib/pdk/util/template_uri.rb @@ -33,7 +33,7 @@ def initialize(opts_or_uri) elsif opts_or_uri.is_a?(Addressable::URI) opts_or_uri.dup else - first_valid_uri(self.class.templates(opts_or_uri)) + self.class.first_valid_uri(self.class.templates(opts_or_uri)) end end @@ -55,10 +55,14 @@ def metadata_format # # @returns String def git_remote - if @uri.is_a?(Addressable::URI) && @uri.fragment - self.class.human_readable(@uri.to_s.chomp('#' + @uri.fragment)) + self.class.git_remote(@uri) + end + + def self.git_remote(uri) + if uri.is_a?(Addressable::URI) && uri.fragment + human_readable(uri.to_s.chomp('#' + uri.fragment)) else - self.class.human_readable(@uri.to_s) + human_readable(uri.to_s) end end @@ -191,10 +195,8 @@ def self.default_template_ref end end - private - # @returns Addressable::URI - def first_valid_uri(templates_array) + def self.first_valid_uri(templates_array) # 1. Get the four sources of URIs # 2. Pick the first non-nil URI # 3. Error if the URI is not a valid git repo (missing directory or http 404) @@ -205,21 +207,20 @@ def first_valid_uri(templates_array) found_template[:uri] end - def valid_template?(template) - return false if template.nil? - return false if template[:uri].nil? + def self.valid_template?(template) + return false if template.nil? || !template.is_a?(Hash) + return false if template[:uri].nil? || !template[:uri].is_a?(Addressable::URI) - repo = if template[:uri].fragment - template[:uri].to_s.chomp("##{template[:uri].fragment}") - else - template[:uri].to_s - end - return true if PDK::Util::Git.repo?(repo) + return true if PDK::Util::Git.repo?(git_remote(template[:uri])) - path = self.class.human_readable(template[:uri].path) + path = human_readable(template[:uri].path) if File.directory?(path) - PDK::Module::TemplateDir.new(path) {} - return true + begin + PDK::Module::TemplateDir.new(path) {} + return true + rescue ArgumentError + nil + end end unless template[:allow_fallback] @@ -229,8 +230,6 @@ def valid_template?(template) end false - rescue ArgumentError - return false end end end diff --git a/spec/unit/pdk/util/template_uri_spec.rb b/spec/unit/pdk/util/template_uri_spec.rb index fb9a05dd8..858435d73 100644 --- a/spec/unit/pdk/util/template_uri_spec.rb +++ b/spec/unit/pdk/util/template_uri_spec.rb @@ -95,10 +95,8 @@ context 'and the answer file template is invalid' do before(:each) do - # rubocop:disable RSpec/AnyInstance - allow_any_instance_of(described_class).to receive(:valid_template?).with(anything).and_call_original - allow_any_instance_of(described_class).to receive(:valid_template?).with(uri: anything, type: anything, allow_fallback: true).and_return(false) - # rubocop:enable RSpec/AnyInstance + allow(described_class).to receive(:valid_template?).with(anything).and_call_original + allow(described_class).to receive(:valid_template?).with(uri: anything, type: anything, allow_fallback: true).and_return(false) end it 'returns the default template' do @@ -388,4 +386,84 @@ end end end + + describe '.valid_template?' do + subject(:return_val) { described_class.valid_template?(template) } + + context 'when passed nil' do + let(:template) { nil } + + it { is_expected.to be_falsey } + end + + context 'when passed a param that is not a Hash' do + let(:template) { 'https://github.com/my/template' } + + it { is_expected.to be_falsey } + end + + context 'when passed a param that is a Hash' do + let(:template) { { allow_fallback: true } } + + context 'with a nil :uri' do + let(:template) { super().merge(uri: nil) } + + it { is_expected.to be_falsey } + end + + context 'and the :uri value is not an Addressable::URI' do + let(:template) { super().merge(uri: 'https://github.com/my/template') } + + it { is_expected.to be_falsey } + end + + context 'and the :uri value is an Addressable::URI' do + let(:template) { super().merge(uri: Addressable::URI.parse('/path/to/a/template')) } + + context 'that points to a git repository' do + before(:each) do + allow(PDK::Util::Git).to receive(:repo?).with('/path/to/a/template').and_return(true) + end + + it { is_expected.to be_truthy } + end + + context 'that does not point to a git repository' do + before(:each) do + allow(PDK::Util::Git).to receive(:repo?).with('/path/to/a/template').and_return(false) + end + + context 'but does point to a directory' do + before(:each) do + allow(File).to receive(:directory?).with('/path/to/a/template').and_return(true) + end + + context 'that contains a valid template' do + before(:each) do + allow(PDK::Module::TemplateDir).to receive(:new).with('/path/to/a/template').and_yield + end + + it { is_expected.to be_truthy } + end + + context 'that does not contain a valid template' do + before(:each) do + allow(PDK::Module::TemplateDir).to receive(:new).with('/path/to/a/template').and_raise(ArgumentError) + end + + it { is_expected.to be_falsey } + end + end + + context 'and the param Hash sets :allow_fallback => false' do + let(:template) { super().merge(allow_fallback: false) } + + it 'raises a FatalError' do + expect { return_val }.to raise_error(PDK::CLI::FatalError, %r{unable to find a valid template}i) + end + end + end + end + end + end end From 3b9a58a198c2362c786f96aca3f834fea6f8f28b Mon Sep 17 00:00:00 2001 From: Tim Sharpe Date: Wed, 20 Feb 2019 15:42:17 +1100 Subject: [PATCH 19/24] Refactor SCP URL handling --- lib/pdk/util/template_uri.rb | 26 +++++------ spec/unit/pdk/util/template_uri_spec.rb | 60 +++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 16 deletions(-) diff --git a/lib/pdk/util/template_uri.rb b/lib/pdk/util/template_uri.rb index dc5cac752..a4bb89163 100644 --- a/lib/pdk/util/template_uri.rb +++ b/lib/pdk/util/template_uri.rb @@ -3,6 +3,8 @@ module PDK module Util class TemplateURI + SCP_PATTERN = %r{\A(?!\w+://)(?:(?.+?)@)?(?[^:/]+):(?.+)\z} + # XXX Previously # - template_uri used to get the string form of the uri when generating the module and written to pdk answers and metadata # - template_path or deuri_path used for humans to see and commands to run @@ -141,23 +143,15 @@ def self.templates(opts) # numbers, but can be made unambiguous by making the form to # ssh://git@github.com/1234/repo.git or # ssh://git@github.com:1234/user/repo.git - scp_url_m = explicit_url.match(%r{\A(.*@[^/:]+):(.+)\z}) - if Pathname.new(explicit_url).relative? && scp_url_m - # "^git@..." is also malformed as it is missing a scheme - # "ssh://git@..." is correct. - check_url = Addressable::URI.parse(scp_url_m[1]) - scheme = 'ssh://' unless check_url.scheme - - numbers_m = scp_url_m[2].split('/')[0].match(%r{\A[0-9]+\z}) - if numbers_m && numbers_m[0].to_i < 65_536 - # consider it an explicit-port URI, even though it's ambiguous. - explicit_url = Addressable::URI.parse(scheme + scp_url_m[1] + ':' + scp_url_m[2]) - else - explicit_url = Addressable::URI.parse(scheme + scp_url_m[1] + '/' + scp_url_m[2]) - PDK.logger.warn(_('--template-url appears to be an SCP-style url; it will be converted to an RFC-compliant URI: %{uri}') % { uri: explicit_url }) - end + scp_url = explicit_url.match(SCP_PATTERN) + if Pathname.new(uri_safe(explicit_url)).relative? && scp_url + explicit_uri = Addressable::URI.new(scheme: 'ssh', user: scp_url[:user], host: scp_url[:host], path: scp_url[:path]) + PDK.logger.warn _('%{scp_uri} appears to be an SCP style URL; it will be converted to an RFC compliant URI: %{rfc_uri}') % { + scp_uri: explicit_url, + rfc_uri: explicit_uri.to_s, + } end - explicit_uri = Addressable::URI.parse(uri_safe(explicit_url)) + explicit_uri ||= Addressable::URI.parse(uri_safe(explicit_url)) explicit_uri.fragment = explicit_ref || default_template_ref else explicit_uri = nil diff --git a/spec/unit/pdk/util/template_uri_spec.rb b/spec/unit/pdk/util/template_uri_spec.rb index 858435d73..35fd541ca 100644 --- a/spec/unit/pdk/util/template_uri_spec.rb +++ b/spec/unit/pdk/util/template_uri_spec.rb @@ -342,6 +342,66 @@ end end + context 'when provided a template-url' do + subject(:cli_template_uri) { described_class.templates(:'template-url' => template_url).first[:uri] } + + context 'that is a ssh:// URL without a port' do + let(:template_url) { 'ssh://git@github.com/1234/repo.git' } + + it 'parses into an Addressable::URI without port set' do + expect(cli_template_uri).to have_attributes( + scheme: 'ssh', + user: 'git', + host: 'github.com', + port: nil, + path: '/1234/repo.git', + ) + end + end + + context 'that is a ssh:// URL with a port' do + let(:template_url) { 'ssh://git@github.com:1234/user/repo.git' } + + it 'parses into an Addressable::URI with port set' do + expect(cli_template_uri).to have_attributes( + scheme: 'ssh', + user: 'git', + host: 'github.com', + port: 1234, + path: '/user/repo.git', + ) + end + end + + context 'that is a SCP style URL with a non-numeric relative path' do + let(:template_url) { 'git@github.com:user/repo.git' } + + it 'parses into an Addressable::URI without port set' do + expect(cli_template_uri).to have_attributes( + scheme: 'ssh', + user: 'git', + host: 'github.com', + port: nil, + path: '/user/repo.git', + ) + end + end + + context 'that is a SCP style URL with a numeric relative path' do + let(:template_url) { 'git@github.com:1234/repo.git' } + + it 'parses the numeric part as part of the path' do + expect(cli_template_uri).to have_attributes( + scheme: 'ssh', + user: 'git', + host: 'github.com', + port: nil, + path: '/1234/repo.git', + ) + end + end + end + context 'when the answers file has saved template-url value' do before(:each) do PDK.answers.update!('template-url' => answers_template_url) From 52cebb8f3cf3691ceeed2f927cd15ff3efe2bcbb Mon Sep 17 00:00:00 2001 From: Tim Sharpe Date: Mon, 18 Mar 2019 13:07:10 +1100 Subject: [PATCH 20/24] Update logic for updating templates between tags If the module being updated is using the default template *and* the template ref is a tag *and* the PDK is being run from a package install, then update the module to use the latest tag of the template. In all other cases, get the ref from the `template_url` metadata value (and if there is no ref in the `template_url` metadata value, use the default template ref which is the latest tag for package installs or `master` otherwise). --- lib/pdk/module/update.rb | 16 ++++-- lib/pdk/util/template_uri.rb | 8 +++ spec/unit/pdk/module/update_spec.rb | 77 +++++++++++++++++++++++++++++ 3 files changed, 96 insertions(+), 5 deletions(-) diff --git a/lib/pdk/module/update.rb b/lib/pdk/module/update.rb index 1ea6a739d..e59702a22 100644 --- a/lib/pdk/module/update.rb +++ b/lib/pdk/module/update.rb @@ -64,6 +64,16 @@ def new_version @new_version ||= fetch_remote_version(new_template_version) end + def new_template_version + return options[:'template-ref'] if options[:'template-ref'] + + if template_uri.default? && template_uri.ref_is_tag? && PDK::Util.package_install? + PDK::Util::TemplateURI.default_template_ref + else + template_uri.git_ref + end + end + private def current_template_version @@ -82,10 +92,6 @@ def describe_ref_to_s(describe_ref) end end - def new_template_version - options.fetch(:'template-ref', PDK::Util::TemplateURI.default_template_ref) - end - def fetch_remote_version(template_ref) return template_ref unless current_template_version.is_a?(String) return template_ref if template_ref == PDK::TEMPLATE_REF @@ -95,7 +101,7 @@ def fetch_remote_version(template_ref) end def update_message - format_string = if template_uri.git_remote == PDK::Util::TemplateURI.default_template_uri.git_remote + format_string = if template_uri.default? _('Updating %{module_name} using the default template, from %{current_version} to %{new_version}') else _('Updating %{module_name} using the template at %{template_url}, from %{current_version} to %{new_version}') diff --git a/lib/pdk/util/template_uri.rb b/lib/pdk/util/template_uri.rb index a4bb89163..62534d789 100644 --- a/lib/pdk/util/template_uri.rb +++ b/lib/pdk/util/template_uri.rb @@ -96,6 +96,14 @@ def self.default_template_uri end end + def default? + git_remote == self.class.default_template_uri.git_remote + end + + def ref_is_tag? + PDK::Util::Git.git('ls-remote', '--tags', '--exit-code', git_remote, git_ref)[:exit_code].zero? + end + # `C:...` urls are not URI-safe. They should be of the form `/C:...` to # be URI-safe. scp-like urls like `user@host:/path` are not URI-safe # either but are not handled here. Should they be? diff --git a/spec/unit/pdk/module/update_spec.rb b/spec/unit/pdk/module/update_spec.rb index 96b66ad43..1311f394b 100644 --- a/spec/unit/pdk/module/update_spec.rb +++ b/spec/unit/pdk/module/update_spec.rb @@ -57,6 +57,7 @@ allow(instance).to receive(:print_summary) allow(instance).to receive(:new_version).and_return('1.4.0') allow(instance).to receive(:print_result) + allow(instance.template_uri).to receive(:ref_is_tag?).and_return(true) allow(instance.update_manager).to receive(:sync_changes!) allow(instance.update_manager).to receive(:changes?).and_return(changes) end @@ -265,4 +266,80 @@ end end end + + describe '#new_template_version' do + subject { described_class.new(options).new_template_version } + + include_context 'with mock metadata' + + let(:module_template_uri) { instance_double(PDK::Util::TemplateURI, default?: true, ref_is_tag?: false, git_ref: '0.0.1') } + let(:template_url) { 'https://github.com/puppetlabs/pdk-templates#0.0.1' } + + before(:each) do + allow(PDK::Util::TemplateURI).to receive(:new).with(template_url).and_return(module_template_uri) + end + + context 'when a template-ref is specified' do + let(:options) { { :'template-ref' => 'my-custom-branch' } } + + it 'returns the specified template-ref value' do + is_expected.to eq('my-custom-branch') + end + end + + context 'when template-ref is not specified' do + context 'and the module is using the default template' do + before(:each) do + allow(module_template_uri).to receive(:default?).and_return(true) + end + + context 'and the ref of the template is a tag' do + before(:each) do + allow(module_template_uri).to receive(:ref_is_tag?).and_return(true) + end + + context 'and PDK is running from a package install' do + before(:each) do + allow(PDK::Util).to receive(:package_install?).and_return(true) + allow(PDK::Util::Version).to receive(:git_ref).and_return('1234acb') + end + + it 'returns the default ref' do + is_expected.to eq(PDK::Util::TemplateURI.default_template_ref) + end + end + + context 'and PDK is not running from a package install' do + before(:each) do + allow(PDK::Util).to receive(:package_install?).and_return(false) + end + + it 'returns the ref from the metadata' do + is_expected.to eq(template_url.split('#').last) + end + end + end + + context 'but the ref of the template is not a tag' do + before(:each) do + allow(module_template_uri).to receive(:ref_is_tag?).and_return(false) + end + + it 'returns the ref from the metadata' do + is_expected.to eq(template_url.split('#').last) + end + end + end + + context 'but the module is not using the default template' do + before(:each) do + allow(module_template_uri).to receive(:default?).and_return(false) + end + + it 'returns the ref stored in the template_url metadata' do + is_expected.to eq(template_url.split('#').last) + end + end + end + end end From 9ef5c2e89266aaa12262139f32c3fcbbdd0d6347 Mon Sep 17 00:00:00 2001 From: Tim Sharpe Date: Mon, 18 Mar 2019 13:48:01 +1100 Subject: [PATCH 21/24] Only display work-tree warning if path is a work tree --- lib/pdk/module/templatedir.rb | 6 +++++- lib/pdk/util/git.rb | 9 +++++++++ spec/unit/pdk/module/template_dir_spec.rb | 5 +++++ 3 files changed, 19 insertions(+), 1 deletion(-) diff --git a/lib/pdk/module/templatedir.rb b/lib/pdk/module/templatedir.rb index d5a9eb5c2..ae70fcf7a 100644 --- a/lib/pdk/module/templatedir.rb +++ b/lib/pdk/module/templatedir.rb @@ -55,7 +55,11 @@ def initialize(uri, module_metadata = {}, init = false) @path = uri.shell_path # We don't do a checkout of local-path repos. There are lots of edge # cases or user un-expectations. - PDK.logger.warn _("Repository '%{repo}' has a work-tree; skipping git reset.") % { repo: @path } + if PDK::Util::Git.work_tree?(@path) + PDK.logger.warn _("Repository '%{repo}' has a work-tree; skipping git reset.") % { + repo: @path, + } + end end @cloned_from = uri.metadata_format diff --git a/lib/pdk/util/git.rb b/lib/pdk/util/git.rb index 0d82d532e..8c56dbe5c 100644 --- a/lib/pdk/util/git.rb +++ b/lib/pdk/util/git.rb @@ -55,6 +55,15 @@ def self.remote_repo?(maybe_repo) git('ls-remote', '--exit-code', maybe_repo)[:exit_code].zero? end + def self.work_tree?(path) + return false unless File.directory?(path) + + Dir.chdir(path) do + rev_parse = git('rev-parse', '--is-inside-work-tree') + rev_parse[:exit_code].zero? && rev_parse[:stdout].strip == 'true' + end + end + def self.work_dir_clean?(repo) raise PDK::CLI::ExitWithError, _('Unable to locate git work dir "%{workdir}') % { workdir: repo } unless File.directory?(repo) raise PDK::CLI::ExitWithError, _('Unable to locate git dir "%{gitdir}') % { gitdir: repo } unless File.directory?(File.join(repo, '.git')) diff --git a/spec/unit/pdk/module/template_dir_spec.rb b/spec/unit/pdk/module/template_dir_spec.rb index bbfa13371..c5860ec30 100644 --- a/spec/unit/pdk/module/template_dir_spec.rb +++ b/spec/unit/pdk/module/template_dir_spec.rb @@ -30,6 +30,11 @@ EOS end + before(:each) do + allow(PDK::Util::Git).to receive(:work_tree?).with(path_or_url).and_return(false) + allow(PDK::Util::Git).to receive(:work_tree?).with(uri.shell_path).and_return(false) + end + describe '.new' do context 'when not passed a block' do it 'raises an ArgumentError' do From 05001adb4d83a4bb1445509bb671593342c15563 Mon Sep 17 00:00:00 2001 From: Tim Sharpe Date: Mon, 18 Mar 2019 16:36:45 +1100 Subject: [PATCH 22/24] Validate template-{url,ref} at the start of CLI runs --- lib/pdk/cli/convert.rb | 2 ++ lib/pdk/cli/module/generate.rb | 2 ++ lib/pdk/cli/new/module.rb | 2 ++ lib/pdk/cli/util.rb | 10 ++++++ lib/pdk/util/template_uri.rb | 7 ---- spec/unit/pdk/cli/convert_spec.rb | 4 +-- spec/unit/pdk/cli/new/module_spec.rb | 9 +++-- spec/unit/pdk/cli/util_spec.rb | 44 +++++++++++++++++++++++++ spec/unit/pdk/util/template_uri_spec.rb | 27 --------------- 9 files changed, 66 insertions(+), 41 deletions(-) diff --git a/lib/pdk/cli/convert.rb b/lib/pdk/cli/convert.rb index 72127101c..1fba522f1 100644 --- a/lib/pdk/cli/convert.rb +++ b/lib/pdk/cli/convert.rb @@ -22,6 +22,8 @@ module PDK::CLI log_level: :info, ) + PDK::CLI::Util.validate_template_opts(opts) + if opts[:noop] && opts[:force] raise PDK::CLI::ExitWithError, _('You can not specify --noop and --force when converting a module') end diff --git a/lib/pdk/cli/module/generate.rb b/lib/pdk/cli/module/generate.rb index afd3232d3..c6265583c 100644 --- a/lib/pdk/cli/module/generate.rb +++ b/lib/pdk/cli/module/generate.rb @@ -20,6 +20,8 @@ module PDK::CLI exit 1 end + PDK::CLI::Util.validate_template_opts(opts) + PDK.logger.info(_('New modules are created using the ‘pdk new module’ command.')) prompt = TTY::Prompt.new(help_color: :cyan) redirect = PDK::CLI::Util::CommandRedirector.new(prompt) diff --git a/lib/pdk/cli/new/module.rb b/lib/pdk/cli/new/module.rb index a0bb7d4e1..3f292b1b7 100644 --- a/lib/pdk/cli/new/module.rb +++ b/lib/pdk/cli/new/module.rb @@ -19,6 +19,8 @@ module PDK::CLI module_name = args[0] target_dir = args[1] + PDK::CLI::Util.validate_template_opts(opts) + if opts[:'skip-interview'] && opts[:'full-interview'] PDK.logger.info _('Ignoring --full-interview and continuing with --skip-interview.') opts[:'full-interview'] = false diff --git a/lib/pdk/cli/util.rb b/lib/pdk/cli/util.rb index 587b284bd..32b11b509 100644 --- a/lib/pdk/cli/util.rb +++ b/lib/pdk/cli/util.rb @@ -188,6 +188,16 @@ def validate_puppet_version_opts(opts) end end module_function :validate_puppet_version_opts + + def validate_template_opts(opts) + if opts[:'template-ref'] && opts[:'template-url'].nil? + raise PDK::CLI::ExitWithError, _('--template-ref requires --template-url to also be specified.') + end + + return unless opts[:'template-url'] && opts[:'template-url'].include?('#') + raise PDK::CLI::ExitWithError, _('--template-url may not be used to specify paths containing #\'s.') + end + module_function :validate_template_opts end end end diff --git a/lib/pdk/util/template_uri.rb b/lib/pdk/util/template_uri.rb index 62534d789..9cecf220e 100644 --- a/lib/pdk/util/template_uri.rb +++ b/lib/pdk/util/template_uri.rb @@ -133,13 +133,6 @@ def self.templates(opts) explicit_url = opts.fetch(:'template-url', nil) explicit_ref = opts.fetch(:'template-ref', nil) - if explicit_ref && explicit_url.nil? - raise PDK::CLI::FatalError, _('--template-ref requires --template-url to also be specified.') - end - if explicit_url && explicit_url.include?('#') - raise PDK::CLI::FatalError, _('--template-url may not be used to specify paths containing #\'s') - end - # 1. Get the CLI, metadata (or answers if no metadata), and default URIs # 2. Construct the hash if explicit_url diff --git a/spec/unit/pdk/cli/convert_spec.rb b/spec/unit/pdk/cli/convert_spec.rb index 3016b310f..fe2112669 100644 --- a/spec/unit/pdk/cli/convert_spec.rb +++ b/spec/unit/pdk/cli/convert_spec.rb @@ -36,9 +36,9 @@ context 'and the --template-ref option has been passed' do it 'invokes the converter with the user supplied template' do - expect(PDK::Module::Convert).to receive(:invoke).with(:'template-url' => anything, :'template-ref' => '1.0.0') + expect(PDK::Module::Convert).to receive(:invoke).with(:'template-url' => 'https://my/template', :'template-ref' => '1.0.0') - PDK::CLI.run(['convert', '--template-url', anything, '--template-ref', '1.0.0']) + PDK::CLI.run(['convert', '--template-url', 'https://my/template', '--template-ref', '1.0.0']) end end diff --git a/spec/unit/pdk/cli/new/module_spec.rb b/spec/unit/pdk/cli/new/module_spec.rb index 0a8c741cb..0c147d615 100644 --- a/spec/unit/pdk/cli/new/module_spec.rb +++ b/spec/unit/pdk/cli/new/module_spec.rb @@ -48,13 +48,12 @@ end end - context 'and the template-ref option' do + context 'and the template-ref without the template-url option' do let(:template_ref) { '1.0.0' } - it 'passes the value of the template-ref option to PDK::Generate::Module.invoke' do - expect(PDK::Generate::Module).to receive(:invoke).with(hash_including(:'template-ref' => template_ref)) - expect(logger).to receive(:info).with("Creating new module: #{module_name}") - PDK::CLI.run(['new', 'module', '--template-ref', template_ref, module_name]) + it 'does not invoke PDK::Generate::Module' do + expect(PDK::Generate::Module).not_to receive(:invoke) + expect { PDK::CLI.run(['new', 'module', '--template-ref', template_ref, module_name]) }.to exit_nonzero end end diff --git a/spec/unit/pdk/cli/util_spec.rb b/spec/unit/pdk/cli/util_spec.rb index 22a333131..7af3ee23b 100644 --- a/spec/unit/pdk/cli/util_spec.rb +++ b/spec/unit/pdk/cli/util_spec.rb @@ -342,6 +342,50 @@ end end + describe '.validate_template_opts' do + subject(:validate_template_opts) { described_class.validate_template_opts(options) } + + let(:options) { {} } + + context 'with no options' do + it 'does not raise an error' do + expect { validate_template_opts }.not_to raise_error + end + end + + context 'when template-ref has been specified but not template-url' do + let(:options) { { :'template-ref' => '1.9.1' } } + + it 'raises an error' do + expect { validate_template_opts }.to raise_error(PDK::CLI::ExitWithError, %r{--template-ref requires --template-url}) + end + end + + context 'when template-ref and template-url have been specified' do + let(:options) { { :'template-url' => 'https://my/template', :'template-ref' => '1.9.1' } } + + it 'does not raise an error' do + expect { validate_template_opts }.not_to raise_error + end + end + + context 'when template-url has been specified but not template-url' do + let(:options) { { :'template-url' => 'https://my/template' } } + + it 'does not raise an error' do + expect { validate_template_opts }.not_to raise_error + end + + context 'and the template-url value contains a #' do + let(:options) { { :'template-url' => 'https://my/template#1.9.1' } } + + it 'raises an error' do + expect { validate_template_opts }.to raise_error(PDK::CLI::ExitWithError, %r{may not be used to specify paths containing #}) + end + end + end + end + describe '.validate_puppet_version_opts' do subject(:validate_puppet_version_opts) { described_class.validate_puppet_version_opts(options) } diff --git a/spec/unit/pdk/util/template_uri_spec.rb b/spec/unit/pdk/util/template_uri_spec.rb index 35fd541ca..5e65c1ba0 100644 --- a/spec/unit/pdk/util/template_uri_spec.rb +++ b/spec/unit/pdk/util/template_uri_spec.rb @@ -137,13 +137,6 @@ expect(template_uri.to_s).to eq("C:\\cli-templates##{described_class.default_template_ref}") end end - context 'and passed template-ref' do - let(:opts_or_uri) { { :'template-ref' => 'cli-ref' } } - - it 'errors because it requires url with ref' do - expect { template_uri }.to raise_error(PDK::CLI::FatalError, %r{--template-ref requires --template-url}) - end - end context 'and passed template-url and template-ref' do let(:opts_or_uri) { { :'template-url' => 'cli-templates', :'template-ref' => 'cli-ref' } } @@ -322,26 +315,6 @@ let(:options) { {} } - context 'when provided a ref but not a url' do - let(:options) { { :'template-ref' => '123' } } - - it 'raises a FatalError' do - expect { - described_class.templates(options) - }.to raise_error(PDK::CLI::FatalError, %r{--template-ref requires --template-url}) - end - end - - context 'when provided a url that contains a hash' do - let(:options) { { :'template-url' => 'https://github.com/puppetlabs/pdk-templates#master' } } - - it 'raises a FatalError' do - expect { - described_class.templates(options) - }.to raise_error(PDK::CLI::FatalError, %r{may not be used to specify paths containing #}i) - end - end - context 'when provided a template-url' do subject(:cli_template_uri) { described_class.templates(:'template-url' => template_url).first[:uri] } From b6a9d585c8f206871ff9a0734b3506393926d0ae Mon Sep 17 00:00:00 2001 From: Tim Sharpe Date: Tue, 19 Mar 2019 14:56:21 +1100 Subject: [PATCH 23/24] (maint) simplify PDK::Util::Git.ls_remote --- lib/pdk/util/git.rb | 11 +++++------ spec/unit/pdk/module/template_dir_spec.rb | 8 ++++---- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/lib/pdk/util/git.rb b/lib/pdk/util/git.rb index 8c56dbe5c..9684c9cc9 100644 --- a/lib/pdk/util/git.rb +++ b/lib/pdk/util/git.rb @@ -72,12 +72,11 @@ def self.work_dir_clean?(repo) end def self.ls_remote(repo, ref) - output = if File.directory?(repo) - # Appveyor doesn't like windows-path repository targets - git('--git-dir', File.join(repo, '.git'), 'ls-remote', '--refs', 'origin', ref) - else - git('ls-remote', '--refs', repo, ref) - end + if File.directory?(repo) + repo = 'file://' + repo + end + + output = git('ls-remote', '--refs', repo, ref) unless output[:exit_code].zero? PDK.logger.error output[:stdout] diff --git a/spec/unit/pdk/module/template_dir_spec.rb b/spec/unit/pdk/module/template_dir_spec.rb index c5860ec30..147294b0a 100644 --- a/spec/unit/pdk/module/template_dir_spec.rb +++ b/spec/unit/pdk/module/template_dir_spec.rb @@ -498,9 +498,9 @@ allow(FileUtils).to receive(:remove_dir).with(tmp_path) allow(PDK::Util::Git).to receive(:git).with('--git-dir', anything, 'describe', '--all', '--long', '--always').and_return(exit_code: 0, stdout: '1234abcd') allow(PDK::Util::Git).to receive(:git).with('--work-tree', anything, '--git-dir', anything, 'status', '--untracked-files=no', '--porcelain', anything).and_return(exit_code: 0, stdout: '') - allow(PDK::Util::Git).to receive(:git).with('--git-dir', anything, 'ls-remote', '--refs', 'origin', 'default-ref').and_return(exit_code: 0, stdout: - "default-sha\trefs/heads/default-ref\n" \ - "default-sha\trefs/remotes/origin/default-ref") + allow(PDK::Util::Git).to receive(:git).with('ls-remote', '--refs', 'file:///tmp/path', 'default-ref').and_return(exit_code: 0, stdout: + "default-sha\trefs/heads/default-ref\n" \ + "default-sha\trefs/remotes/origin/default-ref") allow(PDK::Util::Version).to receive(:version_string).and_return('0.0.0') allow(PDK::Util).to receive(:canonical_path).with(tmp_path).and_return(tmp_path) allow(PDK::Util).to receive(:development_mode?).and_return(false) @@ -526,7 +526,7 @@ allow(FileUtils).to receive(:remove_dir).with(tmp_path) allow(PDK::Util::Git).to receive(:git).with('--git-dir', anything, 'describe', '--all', '--long', '--always').and_return(exit_code: 0, stdout: '1234abcd') allow(PDK::Util::Git).to receive(:git).with('--work-tree', anything, '--git-dir', anything, 'status', '--untracked-files=no', '--porcelain', anything).and_return(exit_code: 0, stdout: '') - allow(PDK::Util::Git).to receive(:git).with('--git-dir', anything, 'ls-remote', '--refs', 'origin', 'default-ref').and_return(exit_code: 0, stdout: + allow(PDK::Util::Git).to receive(:git).with('ls-remote', '--refs', 'file:///tmp/path', 'default-ref').and_return(exit_code: 0, stdout: "default-sha\trefs/heads/default-ref\n" \ "default-sha\trefs/remotes/origin/default-ref") allow(PDK::Util::Version).to receive(:version_string).and_return('0.0.0') From 56f52a19e3727d00d3bbccfa2d1faede0e8940ff Mon Sep 17 00:00:00 2001 From: Tim Sharpe Date: Thu, 21 Mar 2019 15:18:25 +1100 Subject: [PATCH 24/24] Make template-ref metadata more consistent with expectations --- lib/pdk/generate/module.rb | 4 ++-- lib/pdk/module/templatedir.rb | 24 +++++++++++----------- lib/pdk/module/update.rb | 4 ++-- lib/pdk/util/git.rb | 23 +++++++++++++++++++++ lib/pdk/util/template_uri.rb | 2 +- lib/pdk/util/version.rb | 3 +-- spec/unit/pdk/cli/util_spec.rb | 2 +- spec/unit/pdk/module/template_dir_spec.rb | 25 ++++++++++++++++------- spec/unit/pdk/util/version_spec.rb | 2 +- 9 files changed, 61 insertions(+), 28 deletions(-) diff --git a/lib/pdk/generate/module.rb b/lib/pdk/generate/module.rb index 8b20d3a5e..7da2817d4 100644 --- a/lib/pdk/generate/module.rb +++ b/lib/pdk/generate/module.rb @@ -75,7 +75,7 @@ def self.invoke(opts = {}) end # Only update the answers files after metadata has been written. - if template_uri.git_remote == PDK::Util::TemplateURI.default_template_uri.git_remote + if template_uri.default? # If the user specifies our default template url via the command # line, remove the saved template-url answer so that the template_uri # resolution can find new default URLs in the future. @@ -90,7 +90,7 @@ def self.invoke(opts = {}) if FileUtils.mv(temp_target_dir, target_dir) Dir.chdir(target_dir) { PDK::Util::Bundler.ensure_bundle! } unless opts[:'skip-bundle-install'] - PDK.logger.info _('Module \'%{name}\' generated at path \'%{path}\', from template \'%{url}\'.') % { name: opts[:module_name], path: target_dir, url: template_uri.shell_path } + PDK.logger.info _('Module \'%{name}\' generated at path \'%{path}\', from template \'%{url}\'.') % { name: opts[:module_name], path: target_dir, url: template_uri.git_remote } PDK.logger.info(_('In your module directory, add classes with the \'pdk new class\' command.')) end rescue Errno::EACCES => e diff --git a/lib/pdk/module/templatedir.rb b/lib/pdk/module/templatedir.rb index ae70fcf7a..2c8a59a08 100644 --- a/lib/pdk/module/templatedir.rb +++ b/lib/pdk/module/templatedir.rb @@ -47,7 +47,7 @@ def initialize(uri, module_metadata = {}, init = false) if PDK::Util::Git.repo?(uri.git_remote) # This is either a bare local repo or a remote. either way it needs cloning. - @path = self.class.clone_template_repo(uri) + @path = clone_template_repo(uri) temp_dir_clone = true else # if it is a local path & non-bare repo then we can use it directly. @@ -92,16 +92,11 @@ def initialize(uri, module_metadata = {}, init = false) # # @api public def metadata - result = { - 'pdk-version' => PDK::Util::Version.version_string, + { + 'pdk-version' => PDK::Util::Version.version_string, + 'template-url' => @cloned_from, + 'template-ref' => cache_template_ref(@path), } - - result['template-url'] = @cloned_from - - ref_result = PDK::Util::Git.git('--git-dir', File.join(@path, '.git'), 'describe', '--all', '--long', '--always') - result['template-ref'] = ref_result[:stdout].strip if ref_result[:exit_code].zero? - - result end # Loop through the files in the template, yielding each rendered file to @@ -304,7 +299,7 @@ def read_config(loc) # @raise [PDK::CLI::FatalError] If reset HEAD of the cloned repo to desired ref. # # @api private - def self.clone_template_repo(uri) + def clone_template_repo(uri) # @todo When switching this over to using rugged, cache the cloned # template repo in `%AppData%` or `$XDG_CACHE_DIR` and update before # use. @@ -326,10 +321,11 @@ def self.clone_template_repo(uri) end # @api private - def self.checkout_template_ref(path, ref) + def checkout_template_ref(path, ref) if PDK::Util::Git.work_dir_clean?(path) Dir.chdir(path) do full_ref = PDK::Util::Git.ls_remote(path, ref) + cache_template_ref(path, full_ref) reset_result = PDK::Util::Git.git('reset', '--hard', full_ref) return if reset_result[:exit_code].zero? @@ -341,6 +337,10 @@ def self.checkout_template_ref(path, ref) PDK.logger.warn _("Uncommitted changes found when attempting to set HEAD of git repository at '%{repo}' to ref '%{ref}'; skipping git reset.") % { repo: path, ref: ref } end end + + def cache_template_ref(path, ref = nil) + @template_ref ||= PDK::Util::Git.describe(File.join(path, '.git'), ref) + end end end end diff --git a/lib/pdk/module/update.rb b/lib/pdk/module/update.rb index e59702a22..72866cf71 100644 --- a/lib/pdk/module/update.rb +++ b/lib/pdk/module/update.rb @@ -85,8 +85,8 @@ def describe_ref_to_s(describe_ref) return data if data.nil? - if data[:base].start_with?('heads/') - "#{data[:base].gsub(%r{^heads/}, '')}@#{data[:sha]}" + if data[:base] =~ %r{^(?:heads|remotes)/} + "#{data[:base].gsub(%r{^(heads/|remotes/\w+?/)}, '')}@#{data[:sha]}" else data[:base] end diff --git a/lib/pdk/util/git.rb b/lib/pdk/util/git.rb index 9684c9cc9..dca598d9c 100644 --- a/lib/pdk/util/git.rb +++ b/lib/pdk/util/git.rb @@ -1,5 +1,21 @@ module PDK module Util + class GitError < StandardError + attr_reader :stdout + attr_reader :stderr + attr_reader :exit_code + attr_reader :args + + def initialze(args, result) + @args = args + @stdout = result[:stdout] + @stderr = result[:stderr] + @exit_code = result[:exit_code] + + super(_('Git command failed: git %{args}' % { args: args.join(' ') })) + end + end + module Git def self.git_bindir @git_dir ||= File.join('private', 'git', Gem.win_platform? ? 'cmd' : 'bin') @@ -91,6 +107,13 @@ def self.ls_remote(repo, ref) raise PDK::CLI::ExitWithError, _('Unable to find a branch or tag named "%{ref}" in %{repo}') % { ref: ref, repo: repo } if matching_ref.nil? matching_ref.first end + + def self.describe(path, ref = nil) + args = ['--git-dir', path, 'describe', '--all', '--long', '--always', ref].compact + result = git(*args) + raise PDK::Util::GitError, args, result unless result[:exit_code].zero? + result[:stdout].strip + end end end end diff --git a/lib/pdk/util/template_uri.rb b/lib/pdk/util/template_uri.rb index 9cecf220e..a639cf5df 100644 --- a/lib/pdk/util/template_uri.rb +++ b/lib/pdk/util/template_uri.rb @@ -174,7 +174,7 @@ def self.templates(opts) default_uri.fragment = default_template_ref ary = [] - ary << { type: _('--template-url'), uri: explicit_uri, allow_fallback: false } + ary << { type: _('--template-url'), uri: explicit_uri, allow_fallback: false } if explicit_url ary << { type: _('metadata.json'), uri: metadata_uri, allow_fallback: true } if metadata_uri ary << { type: _('PDK answers'), uri: answers_uri, allow_fallback: true } if answers_uri ary << { type: _('default'), uri: default_uri, allow_fallback: false } diff --git a/lib/pdk/util/version.rb b/lib/pdk/util/version.rb index 80bc3799b..993588e35 100644 --- a/lib/pdk/util/version.rb +++ b/lib/pdk/util/version.rb @@ -29,8 +29,7 @@ def self.git_ref return nil unless File.directory?(source_git_dir) - ref_result = PDK::Util::Git.git('--git-dir', source_git_dir, 'describe', '--all', '--long') - return ref_result[:stdout].strip if ref_result[:exit_code].zero? + PDK::Util::Git.describe(source_git_dir) end def self.version_file diff --git a/spec/unit/pdk/cli/util_spec.rb b/spec/unit/pdk/cli/util_spec.rb index 7af3ee23b..30cd48f8e 100644 --- a/spec/unit/pdk/cli/util_spec.rb +++ b/spec/unit/pdk/cli/util_spec.rb @@ -369,7 +369,7 @@ end end - context 'when template-url has been specified but not template-url' do + context 'when template-url has been specified but not template-ref' do let(:options) { { :'template-url' => 'https://my/template' } } it 'does not raise an error' do diff --git a/spec/unit/pdk/module/template_dir_spec.rb b/spec/unit/pdk/module/template_dir_spec.rb index 147294b0a..5699c3eb6 100644 --- a/spec/unit/pdk/module/template_dir_spec.rb +++ b/spec/unit/pdk/module/template_dir_spec.rb @@ -1,3 +1,5 @@ +# rubocop:disable RSpec/AnyInstance + require 'spec_helper' require 'yaml' @@ -118,9 +120,10 @@ allow(PDK::Util).to receive(:package_install?).and_return(true) allow(File).to receive(:fnmatch?).with(anything, path_or_url).and_return(true) allow(PDK::Util).to receive(:package_cachedir).and_return(File.join('/', 'path', 'to', 'package', 'cachedir')) - allow(described_class).to receive(:clone_template_repo).and_return(path_or_url) + allow_any_instance_of(described_class).to receive(:clone_template_repo).and_return(path_or_url) allow(PDK::Util::Git).to receive(:repo?).with(path_or_url).and_return(true) allow(FileUtils).to receive(:remove_dir) + allow(PDK::Util::Git).to receive(:git).with('--git-dir', anything, 'describe', '--all', '--long', '--always', anything).and_return(stdout: 'ref', exit_code: 0) end it 'raises an ArgumentError' do @@ -138,11 +141,19 @@ end end - describe '.checkout_template_ref' do + describe '#checkout_template_ref' do let(:path) { File.join('/', 'path', 'to', 'workdir') } let(:ref) { '12345678' } let(:full_ref) { '123456789abcdef' } + before(:each) do + allow_any_instance_of(described_class).to receive(:clone_template_repo).and_return(path) + allow(PDK::Util::Git).to receive(:repo?).with(anything).and_return(true) + allow(FileUtils).to receive(:remove_dir).with(path) + allow_any_instance_of(described_class).to receive(:validate_module_template!) + allow(PDK::Util::Git).to receive(:describe).and_return('git-ref') + end + context 'when the template workdir is clean' do before(:each) do allow(PDK::Util::Git).to receive(:work_dir_clean?).with(path).and_return(true) @@ -157,7 +168,7 @@ it 'does not raise an error' do expect { - described_class.checkout_template_ref(path, ref) + template_dir.checkout_template_ref(path, ref) }.not_to raise_error end end @@ -173,7 +184,7 @@ expect(logger).to receive(:error).with(result[:stdout]) expect(logger).to receive(:error).with(result[:stderr]) expect { - described_class.checkout_template_ref(path, ref) + template_dir.checkout_template_ref(path, ref) }.to raise_error(PDK::CLI::FatalError, %r{unable to set head of git repository}i) end end @@ -185,7 +196,7 @@ end after(:each) do - described_class.checkout_template_ref(path, ref) + template_dir.checkout_template_ref(path, ref) end it 'warns the user' do @@ -496,7 +507,7 @@ allow(PDK::Util::Git).to receive(:git).with('clone', path_or_url, tmp_path).and_return(exit_code: 0) allow(PDK::Util::Git).to receive(:git).with('reset', '--hard', 'default-sha').and_return(exit_code: 0) allow(FileUtils).to receive(:remove_dir).with(tmp_path) - allow(PDK::Util::Git).to receive(:git).with('--git-dir', anything, 'describe', '--all', '--long', '--always').and_return(exit_code: 0, stdout: '1234abcd') + allow(PDK::Util::Git).to receive(:git).with('--git-dir', anything, 'describe', '--all', '--long', '--always', 'default-sha').and_return(exit_code: 0, stdout: '1234abcd') allow(PDK::Util::Git).to receive(:git).with('--work-tree', anything, '--git-dir', anything, 'status', '--untracked-files=no', '--porcelain', anything).and_return(exit_code: 0, stdout: '') allow(PDK::Util::Git).to receive(:git).with('ls-remote', '--refs', 'file:///tmp/path', 'default-ref').and_return(exit_code: 0, stdout: "default-sha\trefs/heads/default-ref\n" \ @@ -524,7 +535,7 @@ allow(PDK::Util::Git).to receive(:git).with('clone', path_or_url, tmp_path).and_return(exit_code: 0) allow(PDK::Util::Git).to receive(:git).with('reset', '--hard', 'default-sha').and_return(exit_code: 0) allow(FileUtils).to receive(:remove_dir).with(tmp_path) - allow(PDK::Util::Git).to receive(:git).with('--git-dir', anything, 'describe', '--all', '--long', '--always').and_return(exit_code: 0, stdout: '1234abcd') + allow(PDK::Util::Git).to receive(:git).with('--git-dir', anything, 'describe', '--all', '--long', '--always', 'default-sha').and_return(exit_code: 0, stdout: '1234abcd') allow(PDK::Util::Git).to receive(:git).with('--work-tree', anything, '--git-dir', anything, 'status', '--untracked-files=no', '--porcelain', anything).and_return(exit_code: 0, stdout: '') allow(PDK::Util::Git).to receive(:git).with('ls-remote', '--refs', 'file:///tmp/path', 'default-ref').and_return(exit_code: 0, stdout: "default-sha\trefs/heads/default-ref\n" \ diff --git a/spec/unit/pdk/util/version_spec.rb b/spec/unit/pdk/util/version_spec.rb index 06b30f0dd..2ad8c9a82 100644 --- a/spec/unit/pdk/util/version_spec.rb +++ b/spec/unit/pdk/util/version_spec.rb @@ -17,7 +17,7 @@ allow(result).to receive(:[]).with(:stdout).and_return('git_hash') allow(result).to receive(:[]).with(:exit_code).and_return(0) - allow(PDK::Util::Git).to receive(:git).with('--git-dir', %r{.git\Z}, 'describe', '--all', '--long').and_return(result) + allow(PDK::Util::Git).to receive(:git).with('--git-dir', %r{.git\Z}, 'describe', '--all', '--long', '--always').and_return(result) end describe '#git_ref' do