(PDK-1096, PDK-1097, PDK-1098) Add puppet-dev flag to validate and test unit #559
Conversation
bmjen
force-pushed
the
puppet-dev
branch
2 times, most recently
from
b90cad1 to
2c60a29
Compare
spec/unit/pdk/cli/validate_spec.rb
Outdated
| expect { | ||
| PDK::CLI.run(['validate', '--puppet-dev']) | ||
| }.to exit_zero | ||
| end |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Yeah, good point. I was going to address it in the next PR when I do get the puppet-version selection parts tested.
bmjen
changed the title
lib/pdk/util/puppet_version.rb
Outdated
| @@ -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' | |||
There was a problem hiding this comment.
.freeze the dev URL as well?
lib/pdk/util/puppet_version.rb
Outdated
| end | ||
|
|
||
| def puppet_dev_path | ||
| '%{cache}/src/puppet' % { cache: PDK::Util.cachedir } |
lib/pdk/util/puppet_version.rb
Outdated
| 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 |
There was a problem hiding this comment.
Might be worth returning early after the clone since the subsequent fetch is unlikely to bring in any new commits.
lib/pdk/cli/util.rb
Outdated
| 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ah, that's fair. I can make that change and at this part, I'll just call .puppet_dev_env to get the hash.
bmjen
force-pushed
the
puppet-dev
branch
3 times, most recently
from
5cccaa1 to
e13dcc5
Compare
There was a problem hiding this comment.
It'd be good to get coverage of the of the changes in PDK::CLI::Util up to 100% - PDK::CLI::Util.puppet_from_opts_or_env in particular (also some acceptance tests). Other than that, 👍 nice!
Adds the
--puppet-devflag tovalidateandtest unit. This flag will allow users to get Puppet from github source.