(PDK-718) Add --template-ref argument for upstream template repo tags #434

hunner · 2018-02-22T04:43:22Z

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:///<drive>:/... scheme or /<drive>:/ leading-slash to be URI-compatible. I represent paths that lack the file:// scheme as /<drive>:/ internally and strip the leading / when used for commands or serialization in metadata.json.

coveralls · 2018-02-22T04:52:23Z

Coverage increased (+0.1%) to 93.071% when pulling 56f52a1 on hunner:add_template_ref into b512790 on puppetlabs:master.

scotje

Is there a use case for caching the template-ref in the answers file when they are using the default template-url? I could see that leading to problems down the line...

hunner · 2018-02-23T16:18:01Z

I wasn't sure how that should play out at the time. I think you're right though.

I'll rebase once I figure out how this is supposed to work with the templatedir changes in #433.

scotje · 2018-02-23T18:24:31Z

See also PDK-815 where we are probably going to stop comparing template refs directly and instead just see if there are any pending changes.

lib/pdk/module/update.rb

DavidS · 2018-04-04T09:55:17Z

@hunner can you rebase/cleanup this? It would be a great addition to the 1.5 release

hunner · 2018-04-09T21:52:15Z

@scotje

Is there a use case for caching the template-ref in the answers file when they are using the default template-url?

The logic for handling the template-url/template-ref is passed around in various places and morphed by several tickets' changes, so trying to think of how to best make the url/ref work in a consistent manner.

Information for template-* options can come from four places, ordered by priority:

--template-url and --template-ref arguments explicitly by the user (via opts.fetch)
metadata.json stored entries for the existing module (via PDK::Module::Metadata); only the url is useful here as the ref may be set by the pdk itself rather than user-specified.
answers.json stored template-url value for the PDK interview (via PDK.answers)
default value coded into the pdk base (via PDK::Utils for url/ref)

Skip any item when the value is blank.
Skip any template-url that is a missing directory or http 404.
When skipping the template-url due to a missing directory or http 404, the template-ref of that step should also be skipped.
Skipping an answers.json entry should delete the entry as it is no longer reliable.
Explicitly specifying a template-url should also update answers.json and metadata.json.
Whenever template-url is updated in metadata.json then the template-ref should be as well. Inserted if explicit case, otherwise deleted so that the default is used.
template-ref should not be read from answers.json
template-url should be updated in metadata.json when reaching the default case.

At least, this is a stab at solving resolution generically.

DavidS · 2018-04-10T08:29:56Z

In your proposal, there is a slight mix-up between the template-ref the user specifies (a desired state), and the template-ref stored in metadata.json (the current state at the time of the last update or convert).

For improved clarity, I would suggest handling the url and ref always together, and make the template-ref in metadata.json a special case. One way to do this is to fold the ref into the URL fragment (the part after #). That would change @hunner 's list like this:

Information for the template location can come from four places, ordered by priority:

--template-url argument explicitly by the user (via opts.fetch)
metadata.json stored entries for the existing module (via PDK::Module::Metadata); since the url can store the target ref, the template-ref can be ignored here, continuing to store the specific source of the last update
answers.json stored template-url value for the PDK interview (via PDK.answers); can also have an attached #ref
default value coded into the pdk base (via PDK::Utils for url/ref)

Skip any item when the value is blank.
Skip any template-url that is a missing directory or http 404.
Skipping an answers.json entry should delete the entry as it is no longer reliable.
Explicitly specifying a template-url should also update answers.json and metadata.json.
answers.json and metadata.json should always store the actually used URL.
metadata.json's template-ref stores the specific SHA/git describe that was used to last update the module.

The last thing I take issue with is the skip-and-delete ("SAD") on a 404. This is already happening, see e.g. PDK-914, and was quite annoying to me. Consider the situation the user is in after a SAD incident:

Potentially has missed warning message, because processing continued
Having a custom template, using the default instead likely leads to unexpected results on the operation tried
After having fixed the reason why the template was not available, retrying the last pdk command still uses the wrong template.

Instead of SAD, the PDK could throw an error and stop evaluation ("EASE"). Let's consider the user's case after the PDK EASEd them:

Processing has stopped, the error message is the last thing on the screen, given the user a good chance to detect what's wrong
even when using --force, the module would not be touched, as processing has already stopped
After having fixed the reason why the template was not available, retrying the last pdk command still uses the template the user expects.

hunner · 2018-04-11T00:00:04Z

@DavidS Okay. I agree that handling the repo url and ref together is needed, but adding the branch as the url fragment is not part of the git design. This may be odd UX-wise, but looks like it's easier to handle in the code than separatetemplate-url and template-ref resolution interactions.

Would you also propose that users can do a single --template-url /repos/my_repo#alt_branch or --template-url https://github.com/myorg/mytemplates#mybranch argument to specify the url and ref? I think I would prefer that to taking both --template-url and --template-ref cli arguments and combining them in the background. This is UX since code can handle either style.

Using the path#ref format will break if someone uses #'s as part of their repo path directory names like /srv/repo#name or /srv/repos#hunner/myrepo (though #'s are not valid for http urls). We can use url.split('#', 2) to handle #'s as part of the ref name. #SerializationIsHard

I would not want to provide a default ref of origin/master as git upstreams define their own "default branch." pdk update will know what to do after the initial convert/generate. I haven't checked the code if any git commands we use requires a ref or not yet.

As for EASE vs SAD, yes error states are probably better for people to handle.

If the --template-url <something> fails then this is definitely an error.
If the answer.json or metadata.json entry fails when a user is trying to pdk new module or pdk convert or pdk new class then adding a --template-url <something> would update answers/metadata and work.
If the default fails then we have other problems.

DavidS · 2018-04-11T14:16:08Z

Agreed on having separate CLI args to avoid the parsing issues
Agreed on "no ref" meaning "the repo's default branch"
I would still serialize the branch as fragment into the template-url in metadata json, using proper uriencoding to handle #s in the path and the fragement:

[4] pry(main)> URI.parse("http://foo/bar#ref")
=> #<URI::HTTP http://foo/bar#ref>
[5] pry(main)> u = URI.parse("http://foo/bar#ref")
=> #<URI::HTTP http://foo/bar#ref>
[6] pry(main)> u.path
=> "/bar"
[7] pry(main)> u.path = "/bar#bar"
URI::InvalidComponentError: bad component(expected absolute path component): /bar#bar
from /usr/lib/ruby/2.5.0/uri/generic.rb:771:in `check_path'
[8] pry(main)> u.path = "/bar%23bar"
=> "/bar%23bar"
[9] pry(main)> u
=> #<URI::HTTP http://foo/bar%23bar#ref>
[10] pry(main)> u = URI.parse("http://foo/bar#bar#ref")
URI::InvalidURIError: bad URI(is not URI?): http://foo/bar#bar#ref
from /usr/lib/ruby/2.5.0/uri/rfc3986_parser.rb:67:in `split'
[11] pry(main)>

hunner · 2018-04-11T16:09:59Z

@DavidS okay looks like URI can encode the path and handle the fragment for file paths too:

[1] pry(main)> u = URI.parse(URI.encode("file:///foo/bar#test") + "#" + URI.encode("branch#name/test"))
=> #<URI::Generic file:/foo/bar%23test#branch%23name/test>
[2] pry(main)> u.scheme
=> "file"
[3] pry(main)> URI.decode(u.path)
=> "/foo/bar#test"
[4] pry(main)> URI.decode(u.fragment)
=> "branch#name/test"

DavidS · 2018-04-11T17:22:53Z

Perfect.

bodgit · 2018-05-04T12:28:28Z

Would be great to get this merged.

hunner · 2018-05-11T00:01:13Z

This seems to be pretty functional for tag & branch refs of local (absolute paths without a schema) or remote (file:// or http(s):// urls, but I need to fix [email protected]:user/repo urls). It purposefully doesn't work for SHAs (git ls-remote doesn't handle those) but it could. It purposefully doesn't handle #'s in file paths because that's lots of headaches (I should make it raise an error if so, to avoid problems down the road).

Local-dir branch resetting doesn't try to match the branch name to the remote ref, so if the local branch is "master" but --template-url /tmp/pdk-templates --template-ref pin-puppet would cause the local master branch to be reset to the sha of the remote pin-puppet branch rather than creating a local branch called pin-puppet to track the remote one. I'm thinking about just entirely avoiding doing resets for local repos anyway... the /tmp cloning works well enough.

I need to fix rubocop, and write/update lots of specs.

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

bmjen · 2019-03-20T18:54:11Z

lib/pdk/module/templatedir.rb

@@ -82,7 +92,7 @@ def metadata
          'pdk-version' => PDK::Util::Version.version_string,
        }

-        result['template-url'] = @repo ? @repo : @path
+        result['template-url'] = @cloned_from


@rodjek So this seems to still show inconsistency with the latest update. Still seeing this:

"pdk-version": "1.10.0.pre (remotes/hunner/add_template_ref-0-gb6a9d585c8)", "template-url": "https://github.com/puppetlabs/pdk-templates#pin-puppet", "template-ref": "heads/master-0-ge07e4fd"

@scotje @rodjek What's our stance on this? I'm not a fan of the strings that imply "branch" being inconsistent between the template-url and template-ref where one says pin-puppet and the other says heads/master.

scotje

Added a few minor comments but nothing that should block this from being merged!

Checked out the branch and played around a bit and it all seems to work as I would expect. :)

scotje · 2019-03-21T17:40:41Z

lib/pdk/util/template_uri.rb

+          explicit_uri ||= Addressable::URI.parse(uri_safe(explicit_url))
+          explicit_uri.fragment = explicit_ref || default_template_ref
+        else
+          explicit_uri = nil


This else block seems redundant? My understanding is that explicit_url = opts.fetch(:'template-url', nil) above will set the value to nil already in this case.

Yeah, this is kind of unfortunate variable naming explicit_url vs explicit_uri

Ah indeed. That is unfortunate. :)

scotje · 2019-03-21T17:42:06Z

lib/pdk/util/template_uri.rb

+        default_uri.fragment = default_template_ref
+
+        ary = []
+        ary << { type: _('--template-url'), uri: explicit_uri, allow_fallback: false }


Should this one also have a conditional on the end?

Suggested change

ary << { type: _('--template-url'), uri: explicit_uri, allow_fallback: false }

ary << { type: _('--template-url'), uri: explicit_uri, allow_fallback: false } if explicit_uri

I guess it doesn't matter since entries with a nil value for the uri are skipped later, but is there a reason to knowingly add an incomplete entry to this list?

scotje · 2019-03-21T17:45:21Z

lib/pdk/generate/module.rb

-          # 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.git_remote == PDK::Util::TemplateURI.default_template_uri.git_remote


I think this can be simplified now to:

Suggested change

if template_uri.git_remote == PDK::Util::TemplateURI.default_template_uri.git_remote

if template_uri.default?

scotje · 2019-03-21T17:55:08Z

spec/unit/pdk/cli/util_spec.rb

+      end
+    end
+
+    context 'when template-url has been specified but not template-url' do


Suggested change

context 'when template-url has been specified but not template-url' do

context 'when template-url has been specified but not template-ref' do

scotje reviewed Feb 22, 2018

View reviewed changes

scotje added the feature label Feb 22, 2018

scotje mentioned this pull request Mar 7, 2018

Support for template URL with a branch #447

Closed

DavidS changed the title ~~Add --template-ref argument for upstream template repo tags~~ (PDK-718) Add --template-ref argument for upstream template repo tags Apr 4, 2018

DavidS reviewed Apr 4, 2018

View reviewed changes

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

hunner force-pushed the add_template_ref branch 3 times, most recently from c8272af to bc1fc88 Compare April 23, 2018 23:32

hunner force-pushed the add_template_ref branch 2 times, most recently from c2919fc to 2398f71 Compare May 10, 2018 15:35

hunner force-pushed the add_template_ref branch from d909164 to 5bd2543 Compare May 11, 2018 19:14

hunner added the blocked label May 15, 2018

hunner force-pushed the add_template_ref branch 6 times, most recently from 29542d2 to b190dc8 Compare May 18, 2018 17:37

hunner and others added 21 commits March 19, 2019 10:21

(WIP) today's work rewriting template uri handling methods

1b77001

(WIP) More changes today

8a4e6dc

(WIP) Rename things better, start on specs

4c22d55

Specs for shell_path

741fc48

More specs

89fba47

Final specs

78c507c

Fix that I missed

0a9c4ff

Fix spec failures

c7321b2

WIP Tests

eaf9ece

Don't include answers uri if nil

fd0b9e4

Honor --template-ref in pdk update

860f627

Respect allow_fallback when validating template choices

c39ab49

fix rubocop errors

9dcd46a

Consistently include template-ref

8fb4799

clean up after template-ref acceptance tests

ca75938

Validate local templates

9c5aad3

Fill out valid_template? coverage

9081643

Refactor SCP URL handling

3b9a58a

Only display work-tree warning if path is a work tree

9ef5c2e

Validate template-{url,ref} at the start of CLI runs

05001ad

rodjek force-pushed the add_template_ref branch from 35ee2b1 to 05001ad Compare March 18, 2019 23:21

(maint) simplify PDK::Util::Git.ls_remote

b6a9d58

rodjek force-pushed the add_template_ref branch from 380a622 to b6a9d58 Compare March 19, 2019 04:09

bmjen suggested changes Mar 20, 2019

View reviewed changes

bmjen approved these changes Mar 21, 2019

View reviewed changes

scotje approved these changes Mar 21, 2019

View reviewed changes

Make template-ref metadata more consistent with expectations

56f52a1

rodjek force-pushed the add_template_ref branch from 35bc88e to 56f52a1 Compare March 22, 2019 05:09

rodjek merged commit 18721c4 into puppetlabs:master Mar 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(PDK-718) Add --template-ref argument for upstream template repo tags #434

(PDK-718) Add --template-ref argument for upstream template repo tags #434

hunner commented Feb 22, 2018 •

edited

Loading

coveralls commented Feb 22, 2018 •

edited

Loading

scotje left a comment

hunner commented Feb 23, 2018

scotje commented Feb 23, 2018

DavidS commented Apr 4, 2018

hunner commented Apr 9, 2018

DavidS commented Apr 10, 2018

hunner commented Apr 11, 2018

DavidS commented Apr 11, 2018

hunner commented Apr 11, 2018

DavidS commented Apr 11, 2018

bodgit commented May 4, 2018

hunner commented May 11, 2018

bmjen Mar 20, 2019

scotje left a comment

scotje Mar 21, 2019

rodjek Mar 22, 2019

scotje Mar 22, 2019

scotje Mar 21, 2019

scotje Mar 21, 2019

scotje Mar 21, 2019

scotje Mar 21, 2019

	ary << { type: _('--template-url'), uri: explicit_uri, allow_fallback: false }
	ary << { type: _('--template-url'), uri: explicit_uri, allow_fallback: false } if explicit_uri

	if template_uri.git_remote == PDK::Util::TemplateURI.default_template_uri.git_remote
	if template_uri.default?

	context 'when template-url has been specified but not template-url' do
	context 'when template-url has been specified but not template-ref' do

(PDK-718) Add --template-ref argument for upstream template repo tags #434

(PDK-718) Add --template-ref argument for upstream template repo tags #434

Conversation

hunner commented Feb 22, 2018 • edited Loading

coveralls commented Feb 22, 2018 • edited Loading

scotje left a comment

Choose a reason for hiding this comment

hunner commented Feb 23, 2018

scotje commented Feb 23, 2018

DavidS commented Apr 4, 2018

hunner commented Apr 9, 2018

DavidS commented Apr 10, 2018

hunner commented Apr 11, 2018

DavidS commented Apr 11, 2018

hunner commented Apr 11, 2018

DavidS commented Apr 11, 2018

bodgit commented May 4, 2018

hunner commented May 11, 2018

Choose a reason for hiding this comment

scotje 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

Choose a reason for hiding this comment

hunner commented Feb 22, 2018 •

edited

Loading

coveralls commented Feb 22, 2018 •

edited

Loading