-
Notifications
You must be signed in to change notification settings - Fork 106
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
(PF-2332) Add pdk env
subcommand
#957
Conversation
470f0ed
to
071dc97
Compare
Adds an experimental `pdk env` subcommand, which aids in setting a CLI context for a specified version of Puppet.
071dc97
to
a643f92
Compare
The failure in AppVeyor is due to gemfile dependencies, so I don't think it's related to the changes here. |
I clicked rebuild a couple times in Appveyor until it worked. 🤷♂️ |
cc: @jpogran in case he has any feedback about command name or interface |
Oops, looks like I don't have permissions to merge. @scotje do you mind doing the honors? |
allow(PDK::Util::RubyVersion).to receive(:gem_home).and_return('/opt/puppetlabs/pdk/share/cache/ruby/2.4.0') | ||
allow(PDK::Util::RubyVersion).to receive(:gem_path).and_return('/opt/puppetlabs/pdk/private/ruby/2.4.3/lib') | ||
allow(PDK::Util::RubyVersion).to receive(:bin_path).and_return('/opt/puppetlabs/pdk/private/ruby/2.4.3/bin') | ||
allow(PDK::Util::RubyVersion).to receive(:gem_paths_raw).and_return(['/opt/puppetlabs/pdk/private/ruby/2.4.3/lib']) | ||
allow(PDK::Util::Env).to receive(:[]).and_call_original | ||
allow(PDK::Util::Env).to receive(:[]).with('PATH').and_return('/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a concern with using hardcoded nix/macOS type paths here in the spec tests. To me, this should fail becasue those paths do not exist on Windows, but Appveyor is green, so this means either we are not running these tests in appevyor or these tests aren't testing what we think they are?
I realize this is an experimental command, but I don't think this should merge unless we confirm this works on all three platforms. If precedent says PDK doesn't test on windows, then I can add that as a action item for me going forward and can merge this ticket as is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is just mocking the return values and not asserting anything about them, I suppose they could return just about anything, e.g. 'fake return path'
or C:\Windows\Path
. If they aren't mocked, they would presumably return values from the host running the tests, which I thought wasn't totally ideal for a unit test.
To be honest though, I'm not sure of the best way to test this on windows. @scotje do you have any context on PDK and windows testing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@scotje commented that these are unit level tests, so mocking should be OK. I made a mistake and misread these as acceptance level tests. I'll ticket future work to add acceptance level tests and merge as is
Adds an experimental
pdk env
subcommand, which aids in setting a CLI context for a specified version of Puppet.