Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

(PDK-1096, PDK-1097, PDK-1098) Add puppet-dev flag to validate and test unit #559

Merged
merged 7 commits into from
Aug 10, 2018
Merged

(PDK-1096, PDK-1097, PDK-1098) Add puppet-dev flag to validate and test unit #559

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ed4f67d
(PDK-1096) Add puppet-dev flag to CLI
Aug 3, 2018
470ce4c
(PDK-1097, PDK-1098) puppet-dev on backend
Aug 6, 2018
475576d
PR fixups
Aug 7, 2018
d101403
More PR fixups
Aug 8, 2018
ca5dead
tests
Aug 8, 2018
1368053
switch fetch to pull
Aug 9, 2018
db1a57f
acceptance test
Aug 9, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions lib/pdk/cli.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,12 @@ def self.puppet_version_options(dsl)
dsl.option nil, 'pe-version', _('Puppet Enterprise version to run tests or validations against.'), argument: :required
end

def self.puppet_dev_option(dsl)
dsl.option nil,
'puppet-dev',
_('When specified, PDK will validate or test against the current Puppet source from github.com. To use this option, you must have network access to https://github.com.')
end

@base_cmd = Cri::Command.define do
name 'pdk'
usage _('pdk command [options]')
Expand Down
1 change: 1 addition & 0 deletions lib/pdk/cli/test/unit.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ module PDK::CLI
summary _('Run unit tests.')

PDK::CLI.puppet_version_options(self)
PDK::CLI.puppet_dev_option(self)
flag nil, :list, _('List all available unit test files.')
flag nil, :parallel, _('Run unit tests in parallel.')
flag :v, :verbose, _('More verbose --list output. Displays a list of examples in each unit test file.')
Expand Down
31 changes: 29 additions & 2 deletions lib/pdk/cli/util.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,12 +83,16 @@ def module_version_check
module_function :module_version_check

def puppet_from_opts_or_env(opts)
use_puppet_dev = (opts || {})[:'puppet-dev'] || ENV['PDK_PUPPET_DEV']
desired_puppet_version = (opts || {})[:'puppet-version'] || ENV['PDK_PUPPET_VERSION']
desired_pe_version = (opts || {})[:'pe-version'] || ENV['PDK_PE_VERSION']

begin
puppet_env =
if desired_puppet_version
if use_puppet_dev
PDK::Util::PuppetVersion.fetch_puppet_dev
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 think it's optimal for puppet_from_opts_or_env to have the side effect of cloning/fetching the repo every time it's called. Could this be exposed separately and maybe this method can raise if the local repo doesn't exist?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm. I figured it would be appropriate because puppet_from_opts_or_env is called on 3 occasions: pdk validate, pdk test unit, and pdk bundle and the clone or fetch is only performed if --puppet-dev or PDK_PUPPET_DEV is true.

I can abstract the call that clones/fetch the git repo elsewhere, but it would still be called every time puppet-dev is true in order to get the latest from github.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, it currently is a 1-1 correlation with the places you want to call it, but I think it's just unclear that calling that method will have that side effect, so I'd rather see explicit calls in those places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, that's fair. I can make that change and at this part, I'll just call .puppet_dev_env to get the hash.

PDK::Util::PuppetVersion.puppet_dev_env
elsif desired_puppet_version
PDK::Util::PuppetVersion.find_gem_for(desired_puppet_version)
elsif desired_pe_version
PDK::Util::PuppetVersion.from_pe_version(desired_pe_version)
Expand Down Expand Up @@ -132,6 +136,21 @@ def validate_puppet_version_opts(opts)
pe_ver_specs << '--pe-version option' if opts[:'pe-version']
pe_ver_specs << 'PDK_PE_VERSION environment variable' if ENV['PDK_PE_VERSION'] && !ENV['PDK_PE_VERSION'].empty?

puppet_dev_specs = []
puppet_dev_specs << '--puppet-dev flag' if opts[:'puppet-dev']
puppet_dev_specs << 'PDK_PUPPET_DEV environment variable' if ENV['PDK_PUPPET_DEV'] && !ENV['PDK_PUPPET_DEV'].empty?

puppet_dev_specs.each do |pup_dev_spec|
[puppet_ver_specs, pe_ver_specs].each do |offending|
next if offending.empty?

raise PDK::CLI::ExitWithError, _('You cannot specify a %{first} and %{second} at the same time') % {
first: pup_dev_spec,
second: offending.first,
}
end
end

puppet_ver_specs.each do |pup_ver_spec|
next if pe_ver_specs.empty?

Expand All @@ -143,7 +162,15 @@ def validate_puppet_version_opts(opts)
}
end

if puppet_ver_specs.size == 2
if puppet_dev_specs.size == 2
warning_str = 'Puppet dev flag from command line: "--puppet-dev" '
warning_str += 'overrides value from environment: "PDK_PUPPET_DEV=true". You should not specify both.'

PDK.logger.warn(_(warning_str) % {
pup_ver_opt: opts[:'puppet-dev'],
pup_ver_env: ENV['PDK_PUPPET_DEV'],
})
elsif puppet_ver_specs.size == 2
warning_str = 'Puppet version option from command line: "--puppet-version=%{pup_ver_opt}" '
warning_str += 'overrides value from environment: "PDK_PUPPET_VERSION=%{pup_ver_env}". You should not specify both.'

Expand Down
1 change: 1 addition & 0 deletions lib/pdk/cli/validate.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ module PDK::CLI
)

PDK::CLI.puppet_version_options(self)
PDK::CLI.puppet_dev_option(self)
flag nil, :list, _('List all available validators.')
flag :a, 'auto-correct', _('Automatically correct problems where possible.')
flag nil, :parallel, _('Run validations in parallel.')
Expand Down
35 changes: 34 additions & 1 deletion lib/pdk/util/puppet_version.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
require 'pdk/util'
require 'pdk/util/git'

module PDK
module Util
class PuppetVersion
class << self
extend Forwardable

def_delegators :instance, :find_gem_for, :from_pe_version, :from_module_metadata, :latest_available
def_delegators :instance, :puppet_dev_env, :puppet_dev_path, :fetch_puppet_dev, :find_gem_for, :from_pe_version, :from_module_metadata, :latest_available

attr_writer :instance

Expand All @@ -16,6 +17,19 @@ def instance
end

PE_VERSIONS_URL = 'https://forgeapi.puppet.com/private/versions/pe'.freeze
DEFAULT_PUPPET_DEV_URL = 'https://github.com/puppetlabs/puppet'.freeze
DEFAULT_PUPPET_DEV_BRANCH = 'master'.freeze

def puppet_dev_env
{
gem_version: 'file://%{path}' % { path: puppet_dev_path },
ruby_version: PDK::Util::RubyVersion.default_ruby_version,
}
end

def puppet_dev_path
'%{cache}/src/puppet' % { cache: PDK::Util.cachedir }
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this use File.join?

end

def latest_available
latest = find_gem(Gem::Requirement.create('>= 0'))
Expand All @@ -27,6 +41,25 @@ def latest_available
latest
end

def fetch_puppet_dev
unless PDK::Util::Git.remote_repo? puppet_dev_path
FileUtils.mkdir_p puppet_dev_path
clone_result = PDK::Util::Git.git('clone', DEFAULT_PUPPET_DEV_URL, puppet_dev_path)
unless clone_result[:exit_code].zero?
PDK.logger.error clone_result[:stdout]
PDK.logger.error clone_result[:stderr]
raise PDK::CLI::FatalError, _("Unable to clone git repository at '%{repo}'.") % { repo: DEFAULT_PUPPET_DEV_URL }
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worth returning early after the clone since the subsequent fetch is unlikely to bring in any new commits.

end

fetch_result = PDK::Util::Git.git('fetch', '--quiet', puppet_dev_path)
return if fetch_result[:exit_code].zero?

PDK.logger.error fetch_result[:stdout]
PDK.logger.error fetch_result[:stderr]
raise PDK::CLI::FatalError, _("Unable to fetch updates for git repository at '%{cachedir}'.") % { repo: puppet_dev_path }
end

def find_gem_for(version_str)
version = parse_specified_version(version_str)

Expand Down
28 changes: 28 additions & 0 deletions spec/unit/pdk/cli/validate_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,4 +221,32 @@
}.to exit_zero
end
end

context 'with --puppet-dev' do
it 'activates resolved puppet version' do
expect {
PDK::CLI.run(['validate', '--puppet-dev'])
}.to exit_zero
end
Copy link
Contributor

Choose a reason for hiding this comment

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

This test isn't actually asserting anything about activating the right puppet version yet, I assume this that part hasn't been implemented in this PR. But maybe it would be better to leave this out or mark it as pending so that we don't accidentally leave it this way later?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, good point. I was going to address it in the next PR when I do get the puppet-version selection parts tested.

end

context 'with both --puppet-version and --puppet-dev' do
it 'exits with an error' do
expect(logger).to receive(:error).with(a_string_matching(%r{cannot specify.*--puppet-dev.*and.*--puppet-version}i))

expect {
PDK::CLI.run(%w[validate --puppet-version 4.10.10 --puppet-dev])
}.to exit_nonzero
end
end

context 'with both --pe-version and --puppet-dev' do
it 'exits with an error' do
expect(logger).to receive(:error).with(a_string_matching(%r{cannot specify.*--puppet-dev.*and.*--pe-version}i))

expect {
PDK::CLI.run(%w[validate --pe-version 2018.1 --puppet-dev])
}.to exit_nonzero
end
end
end