Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(PDK-1354) Default template ref for custom templates should always be master #677

Merged
merged 1 commit into from
Jun 13, 2019
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
(PDK-1354) Default template ref for custom templates should always be…
… master
rodjek committed Jun 13, 2019

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
commit cb4574baf523162e1c9cb0bd3ebabf6100e42365
18 changes: 9 additions & 9 deletions lib/pdk/util/template_uri.rb
Original file line number Diff line number Diff line change
@@ -96,7 +96,7 @@ def shell_path

# @returns String
def git_ref
@uri.fragment || self.class.default_template_ref
@uri.fragment || self.class.default_template_ref(self)
end

def git_ref=(ref)
@@ -175,7 +175,7 @@ def self.templates(opts)
# 2. Construct the hash
if explicit_url
explicit_uri = Addressable::URI.parse(uri_safe(explicit_url))
explicit_uri.fragment = explicit_ref || default_template_ref
explicit_uri.fragment = explicit_ref || default_template_ref(new(explicit_uri))
else
explicit_uri = nil
end
@@ -196,7 +196,7 @@ def self.templates(opts)
nil
end
default_uri = default_template_uri.uri
default_uri.fragment = default_template_ref
default_uri.fragment = default_template_ref(default_template_uri)

ary = []
ary << { type: _('--template-url'), uri: explicit_uri, allow_fallback: false } if explicit_url
@@ -207,12 +207,12 @@ def self.templates(opts)
end

# @returns String
def self.default_template_ref
if PDK::Util.development_mode?
'master'
else
PDK::TEMPLATE_REF
end
def self.default_template_ref(uri = nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems a little complex. Maybe this is better (not sure)?

def self.default_template_ref(uri = nil)
  return 'master' if PDK::Util.development_mode?
  return PDK::TEMPLATE_REF if uri.nil?  # The default url always uses PDK::TEMPLATE_REF as the ref
  uri = new(uri) unless uri.is_a?(self)
  uri.default? ? PDK::TEMPLATE_REF : 'master'
end

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I think @glennsarti 's guard clause version is clearer.

return 'master' if PDK::Util.development_mode?
return PDK::TEMPLATE_REF if uri.nil?

uri = new(uri) unless uri.is_a?(self)
uri.default? ? PDK::TEMPLATE_REF : 'master'
end

# @returns Addressable::URI
2 changes: 2 additions & 0 deletions spec/unit/pdk/module/update_spec.rb
Original file line number Diff line number Diff line change
@@ -276,6 +276,7 @@
let(:template_url) { 'https://github.com/puppetlabs/pdk-templates#0.0.1' }

before(:each) do
allow(PDK::Util::TemplateURI).to receive(:new).and_call_original
allow(PDK::Util::TemplateURI).to receive(:new).with(template_url).and_return(module_template_uri)
end

@@ -302,6 +303,7 @@
before(:each) do
allow(PDK::Util).to receive(:package_install?).and_return(true)
allow(PDK::Util::Version).to receive(:git_ref).and_return('1234acb')
allow(PDK::Util).to receive(:package_cachedir).and_return(File.join('package', 'cachedir'))
end

it 'returns the default ref' do
62 changes: 45 additions & 17 deletions spec/unit/pdk/util/template_uri_spec.rb
Original file line number Diff line number Diff line change
@@ -152,16 +152,16 @@
let(:opts_or_uri) { { :'template-url' => 'cli-templates' } }

it 'returns the specified template' do
expect(template_uri.to_s).to eq("cli-templates##{described_class.default_template_ref}")
expect(template_uri.to_s).to eq('cli-templates#master')
end
end

context 'and passed windows template-url' do
let(:opts_or_uri) { { :'template-url' => 'C:\cli-templates' } }
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##{described_class.default_template_ref}")
expect(template_uri.to_s).to eq('C:\\cli-templates#master')
end
end

@@ -229,7 +229,7 @@
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
expect(template_uri.git_ref).to eq(described_class.default_template_ref(opts_or_uri))
end
end
end
@@ -302,37 +302,65 @@
end

describe '.default_template_ref' do
subject { described_class.default_template_ref }
subject { described_class.default_template_ref(uri) }
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see any tests for described_class.default_template_ref() anywhere. Should these be added?

Copy link
Contributor

Choose a reason for hiding this comment

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

The only thing I would be looking for is:
expect(described_class.default_template_ref()).to eq(PDK::TEMPLATE_REF)


before(:each) do
allow(PDK::Util).to receive(:development_mode?).and_return(development_mode)
end

context 'with a custom template repo' do
before(:each) do
allow(described_class).to receive(:default_template_url).and_return('custom_template_url')
let(:uri) { described_class.new('https://github.com/my/template') }

context 'in development mode' do
let(:development_mode) { true }

it 'returns master' do
is_expected.to eq('master')
end
end

it 'returns master' do
is_expected.to eq('master')
context 'not in development mode' do
let(:development_mode) { false }

it 'returns master' do
is_expected.to eq('master')
end
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
let(:uri) { described_class.default_template_uri }

context 'not in development mode' do
before(:each) do
allow(PDK::Util).to receive(:development_mode?).and_return(false)
end
let(:development_mode) { false }

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(PDK::Util).to receive(:development_mode?).and_return(true)
let(:development_mode) { true }

it 'returns master' do
is_expected.to eq('master')
end
end
end

context 'with an explicit nil template' do
let(:uri) { nil }

context 'not in development mode' do
let(:development_mode) { false }

it 'returns the built-in TEMPLATE_REF' do
is_expected.to eq(PDK::TEMPLATE_REF)
end
end

context 'in development mode' do
let(:development_mode) { true }

it 'returns master' do
is_expected.to eq('master')