(PDK-1364) Allow non-git template directories to be used #803

glennsarti · 2019-11-14T07:33:20Z

Previously the PDK could only use git based template directories which made
testing new templates difficult and non-intuitive. This commit makes the
following changes which enables plain file system based directories to be used
as a PDK template during operations like PDK update and PDK convert:

Changes TemplateURI so that is now a pure URI. Previously many git type
methods and functions where also mixed in, which muddied what the class
actually did.
- The Git functionality is moved to the PDK::Util::Git module
- git_remote is now bare_uri (A URI without a fragment)
- git_ref is now uri_fragment
Moves the default template url methods in TemplateURI from class instance to
module instance. This stops the expensive TemplateDir class being instantiated
more than once
Move the TemplateDir validate_module_template! from a class instance to a
module instance method. This means that we do not have to create the very
expensive TemplateDir object, and can just quickly validate a local path as a
template directory.
Splits out the default default_template_uri method to default_template_uri and
default_template_addressable_uri. This stops needless creations of TemplateURI
objects.
The default default_template_uri method no longer takes a ref parameter, as it
should only return a URI, not a URI and ref.
The TemplateURI now has no knowledge of what it's pointing to. The
TemplateDir class has been updated to detect what kind of Template the URI is
pointing to. This kind is then stored in the new template_dir_type variable
which then modifies the behavior of the class. For example, the metadata now
outputs the template-ref for Git based templates as it makes no sense for
a non-git template directory.

coveralls · 2019-11-14T07:52:38Z

Coverage increased (+0.07%) to 91.862% when pulling aa161ce on glennsarti:use-template-filesystem into 8e4bcf8 on puppetlabs:master.

Files in the tmp directory are ignored in Git but not from rubocop. This commit excludes tmp from rubocop to be consistent.

Quering if a directory is a git repository is an expensive call, particularly on Windows. This commit adds a caching layer to the repo? and work_tree? methods that caches the results for a short amount of time (GIT_QUERY_CACHE_TTL). These methods are commonly called multiple times when doing PDK Update and PDK Convert operations (at least twice). This commit also updates the repo? method to be consistent with its name. Previously it would return false for local directories that have a working tree. With this change it will detect if a directory is any kind of git repository.

lib/pdk/module/templatedir.rb

rodjek

Looks good so far! 👍 As we discussed in chat, it'd be a good idea in terms of cleanliness to subclass TemplateDir to handle the exclusive git vs filesystem logic.

lib/pdk/module/templatedir.rb

Previously the PDK could only use git based template directories which made testing new templates difficult and non-intuitive. This commit makes the following changes which enables plain file system based directories to be used as a PDK template during operations like PDK update and PDK convert: * Changes TemplateURI so that is now a pure URI. Previously many git type methods and functions where also mixed in, which muddied what the class actually did. - The Git functionality is moved to the PDK::Util::Git module - git_remote is now bare_uri (A URI without a fragment) - git_ref is now uri_fragment * Moves the default template url methods in TemplateURI from class instance to module instance. This stops the expensive TemplateDir class being instantiated more than once * Move the TemplateDir validate_module_template! from a class instance to a module instance method. This means that we do not have to create the very expensive TemplateDir object, and can just quickly validate a local path as a template directory. * Splits out the default default_template_uri method to default_template_uri and default_template_addressable_uri. This stops needless creations of TemplateURI objects. * The default default_template_uri method no longer takes a ref parameter, as it should only return a URI, not a URI and ref. * The TemplateURI now has no knowledge of _what_ it's pointing to. The TemplateDir class has been updated to detect what kind of Template the URI is pointing to. This kind is then stored in the new template_dir_type variable which then modifies the behavior of the class. For example, the metadata now outputs the template-ref for Git based templates as it makes no sense for a non-git template directory.

This commit adds tests for plain filesystem based templates. Namely that they can be detected and that the TemplateDir object responds with the correct metadata information

Previously the TemplateDir was modified to behave differently between a Git and local filesystem. This commit refactors TemplateDir into separate classes: - Base for Common methods - Git to override for Git specific behaviour - Local to override for Git specific behaviour The PDK::Module::TemplateDir is now a module instead of class and uses a factory style method of new to create the appropriate TemplateDir class THe tests have also been split into their new respective files.

Previosuly TemplateDir was a class, but is now a module. While it is still possible to use a method called `new` this is confusing as it looks like a normal object instantiation call. This commit updates the method name to .with which is a much better description of what the method does.

The templatedir.rb file has the TemplateDir module in there however, the filename should really be template_dir.rb according to ruby naming standards. This commit renames the file and updates any locations which use it.

lib/pdk/module/template_dir.rb

lib/pdk/module/template_dir/base.rb

rodjek · 2019-11-27T03:44:29Z

spec/unit/pdk/generate/module_spec.rb

@@ -29,11 +29,13 @@
 end

 shared_context 'mock template dir' do
-  let(:test_template_dir) { instance_double(PDK::Module::TemplateDir, metadata: {}) }
+  require 'pdk/module/template_dir/base'


Rather than adding these requires into the test, it would be better to add autoloads for the subclasses at the top of lib/pdk/module/template_dir.rb

Maybe? Wasn't sure it was worth it as it's only in the spec.

This commit updates the error messages to be more descriptive of what caused the error.

scotje · 2019-11-22T20:33:51Z

lib/pdk/module/template_dir.rb

+          require 'pdk/util'
+
+          if PDK::Util.package_install? && PDK::Util::Filesystem.fnmatch?(File.join(PDK::Util.package_cachedir, '*'), template_root_dir)
+            raise ArgumentError, _('The built-in template has substantially changed. Please run "pdk convert" on your module to continue.')


Is the situation here that they must have an old template_root_dir in their metadata and that template path no longer exists in the package's cache area?

Unknown. 🤷‍♂️ This is a file rename from https://github.com/puppetlabs/pdk/blob/61bb2fc6b2/lib/pdk/module/templatedir.rb#L215-L219

Gotcha, well we won't worry about it right now then. :)

scotje · 2019-11-22T20:44:35Z

lib/pdk/util/git.rb

@@ -19,6 +19,8 @@ def initialze(args, result)
    end

    module Git
+      GIT_QUERY_CACHE_TTL ||= 10


I'm curious what the cases are where it's not safe to just cache the results for the remaining duration of execution of a PDK command, but it is safe to cache it for 10 seconds?

The use case I was worried about was:

Check if a directory, which doesn't exist, is a git repo (e.g. /foo)

Then git clone into that directory (git clone ...uri... /foo)

foo is NOW a valid git repo.

Note that .clone isn't a method in the Git module.

Yeah I'm not 100% happy with using an "arbitrary" time out either, but I couldn't figure out any other way to invalidate the cache. There are significant improvements with caching though.

10s was, in my mind, long enough to provide a performance benefit but slow enough so that it would be meaningful.

Open to other ideas on how to do this 👍

I see.

I would say that down the line it might be good to route all git operations through this module and then the methods that impact the validity of the cache can just handle the invalidation internally as part of like PDK::Util::Git.clone or whatever. Perhaps we should just add a maintenance ticket to centralize all Git operations like we did for filesystem stuff?

Raised https://tickets.puppetlabs.com/browse/PDK-1551 to track the work

scotje · 2019-12-02T23:52:29Z

lib/pdk/util/git.rb

@@ -81,10 +90,12 @@ def self.remote_repo?(maybe_repo)

      def self.work_tree?(path)
        return false unless PDK::Util::Filesystem.directory?(path)
+        result = cached_git_query(path, :work_tree?)
+        return result unless result.nil?


Is it too much optimization to check the cache before hitting the filesystem with .directory? here?

I felt it was. and as per the workflow above, if the directory is later created then we'd get false cache hits.

glennsarti requested a review from a team as a code owner November 14, 2019 07:33

glennsarti added blocked WIP and removed blocked labels Nov 14, 2019

glennsarti force-pushed the use-template-filesystem branch from bc179ba to e33236b Compare November 18, 2019 03:40

glennsarti added 2 commits November 18, 2019 15:27

(maint) Exclude tmp from rubocop

042b65e

Files in the tmp directory are ignored in Git but not from rubocop. This commit excludes tmp from rubocop to be consistent.

glennsarti force-pushed the use-template-filesystem branch 2 times, most recently from ceeadde to da1c0c2 Compare November 18, 2019 08:04

glennsarti commented Nov 19, 2019

View reviewed changes

lib/pdk/module/templatedir.rb Outdated Show resolved Hide resolved

rodjek reviewed Nov 19, 2019

View reviewed changes

lib/pdk/module/templatedir.rb Outdated Show resolved Hide resolved

glennsarti added 3 commits November 19, 2019 13:37

(PDK-1364) Add tests for filesystem based templates

b4bb996

This commit adds tests for plain filesystem based templates. Namely that they can be detected and that the TemplateDir object responds with the correct metadata information

glennsarti force-pushed the use-template-filesystem branch from da1c0c2 to f7c8e3a Compare November 19, 2019 07:38

glennsarti added 2 commits November 22, 2019 14:14

(PDK-1364) Rename templatedir.rb

ff93d12

The templatedir.rb file has the TemplateDir module in there however, the filename should really be template_dir.rb according to ruby naming standards. This commit renames the file and updates any locations which use it.

glennsarti changed the title ~~{WIP} (PDK-1634) Allow non-git template directories to be used~~ (PDK-1364) Allow non-git template directories to be used Nov 22, 2019

glennsarti removed the WIP label Nov 25, 2019

rodjek reviewed Nov 27, 2019

View reviewed changes

glennsarti force-pushed the use-template-filesystem branch from aa161ce to 7985c0e Compare November 27, 2019 08:20

(PDK-1363) Update error messages

4ffd580

This commit updates the error messages to be more descriptive of what caused the error.

glennsarti force-pushed the use-template-filesystem branch from 7985c0e to 4ffd580 Compare November 27, 2019 08:23

rodjek approved these changes Dec 1, 2019

View reviewed changes

scotje reviewed Dec 2, 2019

View reviewed changes

scotje approved these changes Dec 3, 2019

View reviewed changes

glennsarti merged commit 08187eb into puppetlabs:master Dec 4, 2019

rodjek added the feature label Dec 13, 2019

glennsarti deleted the use-template-filesystem branch March 5, 2020 04:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(PDK-1364) Allow non-git template directories to be used #803

(PDK-1364) Allow non-git template directories to be used #803

glennsarti commented Nov 14, 2019 •

edited

Loading

coveralls commented Nov 14, 2019 •

edited

Loading

rodjek left a comment

rodjek Nov 27, 2019

glennsarti Nov 27, 2019

scotje Nov 22, 2019

glennsarti Dec 3, 2019

scotje Dec 3, 2019

scotje Nov 22, 2019

glennsarti Dec 3, 2019 •

edited

Loading

scotje Dec 3, 2019

glennsarti Dec 4, 2019

scotje Dec 2, 2019

glennsarti Dec 3, 2019

(PDK-1364) Allow non-git template directories to be used #803

(PDK-1364) Allow non-git template directories to be used #803

Conversation

glennsarti commented Nov 14, 2019 • edited Loading

coveralls commented Nov 14, 2019 • edited Loading

rodjek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glennsarti Dec 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glennsarti commented Nov 14, 2019 •

edited

Loading

coveralls commented Nov 14, 2019 •

edited

Loading

glennsarti Dec 3, 2019 •

edited

Loading