(SDK-261) Manage basic bundler operations for module dev #62

scotje · 2017-06-05T22:01:56Z

This seems to be working pretty well at this point.

I added a conditional to disable the spinner stuff on Windows for now because there are some special considerations for making control sequences work under cmd/ps. Definitely solvable but probably more appropriate for a subsequent release.

There are a couple PRs backed up in the packaging repo to make this all work with packaged builds, but the paths etc. are all up to date with where things will eventually be.

rodjek · 2017-06-06T00:50:08Z

lib/pdk/util/bundler.rb

+        def installed?
+          output_start(_("Checking for missing Gemfile dependencies"))
+
+          result = invoke('check', "--gemfile=#{gemfile}", "--path=#{pdk_cachedir}")


I'd recommend changing the name of pdk_cachedir to something like pdk_gemdir to avoid confusion. In Bundler cache generally refers to the contents of vendor/cache, as opposed to the value of --path which is where the bundled gems are installed to.

Yeah, right now it points to a generic directory we can use for any sort of caching we need to do with PDK and the default gem pathing of ruby/2.1.0/... gets added to the passed in value by bundler, so the value that is getting interpolated there isn't really specific to gems.

👍 to having a general cache dir for the PDK. To prevent possible conflicts, I think we should standardise on #{pdk_cachedir}/<toolname> (in case some other tool that we want to integrate also wants to create a ruby directory in its cache for example).

DavidS

Tried running the pdk test unit command on a newly generated module. Killed the bundle install process. Good: output of failing command is displayed.
Bad: no indication of what went wrong, still tried running unit tests.

[✔] Checking for missing Gemfile dependencies
[✖] Installing missing Gemfile dependencies
Fetching gem metadata from https://rubygems.org/..........
Fetching version metadata from https://rubygems.org/..
Resolving dependencies........
Installing rake 12.0.0
Installing CFPropertyList 2.2.8
Installing public_suffix 2.0.5
...
Installing win32-eventlog 0.6.5
Installing win32-process 0.7.5
Installing win32-security 0.2.5
Running unit tests: 
$

Looking back into the BundleHelper, I wonder if it might be beneficial to move the spinner and output handling to the PDK::CLI::Exec, to make it available to all commands?

DavidS · 2017-06-06T08:35:19Z

lib/pdk/cli/exec.rb

@@ -46,6 +46,13 @@ def self.git(*args)
        git_path = ENV['PDK_USE_SYSTEM_BINARIES'].nil? ? File.join(git_bindir, 'git') : 'git'
        execute(git_path, *args)
      end
+
+      def self.bundle(*args)
+        bundle_path = ENV['PDK_USE_SYSTEM_BINARIES'].nil? ? File.join(pdk_basedir, 'private', 'ruby', '2.1.9', 'bin', 'bundle') : 'bundle'


This conflicts with 2d0ca92 from #58 - maybe I should pull this change out to get it merged independently, so we can leave the PDK_USE_SYSTEM_BINARIES behind us.

This got merged, so you can just use the fallback code from there.

Yes, I was planning to adopt that approach once it got merged.

DavidS · 2017-06-06T08:37:41Z

lib/pdk/util/bundler.rb

+        def lock
+          output_start(_("Resolving Gemfile dependencies"))
+
+          result = invoke('lock')


You're passing --gemfile to other bundler invocations. shouldn't this one too?

For whatever reason, bundle lock doesn't have a --gemfile option.

DavidS · 2017-06-06T08:41:03Z

spec/util/bundler_spec.rb

+        allow(File).to receive(:file?).with(/Gemfile$/).and_return(false)
+      end
+
+      it 'does nothing' do


In which situations do you expect this to be the case?

Well, technically a puppet module doesn't have to have a Gemfile right? Theoretically somebody could still use pdk to run lint or something on such a module. Did we decide to ignore any use cases that did not involve a module that was generated with pdk new?

Yes, technically no Gemfile is required. Practically, all well-maintained do.

The goal is to provide a template so that pdk new generated modules look very similar to what is currently "on the market", to the point where the pdk can handle unmodified supported and voxpupuli modules.

For less well maintained modules, having a pdk upgrade command applying a template on top of an existing module provides more value, as it'll reduce long-term friction of supporting/working with multiple models.

I guess we just need to figure out how the most graceful way to handle the lack of a Gemfile is. We can change the method name to something like require_bundle! to make it clear that the tool should exit 1 if there is no Gemfile present in the module.

DavidS · 2017-06-06T08:43:05Z

spec/util/bundler_spec.rb

+      end
+
+      it 'generates Gemfile.lock' do
+        expect(PDK::CLI::Exec).to receive(:execute).with(/bundle(\.bat)?$/, 'lock', any_args).and_return({exit_code: 0})


You can save yourself some regexing here by expecting on PDK::CLI::Exec.bundle, instead of .exec, and test the path resolver separately. This will also make changing the grizzards of PDK::CLI::Exec much easier on the tests.

Yeah that would be simpler at this point, I think the original tests were written before the refactor that added the PDK::CLI::Exec.bundle method.

DavidS · 2017-06-06T12:05:07Z

With #58 merged, it would also be good to start with a bit of acceptance testing, to get into the swing of it. @james-stocks and I talked about negative test cases yesterday, and - since we're integrating a third party tool - we don't need to go overboard here. Checking whether our integration reacts correctly to a removed Gemfile.lock, and a broken Gemfile is probably enough.

scotje · 2017-06-06T16:58:03Z

@DavidS Yes we should make the bundler stuff exit on failure so that it doesn't proceed onto the unit test stuff. I also agree that we could move the spinner stuff out into the Exec class but I would prefer to do that in a new PR.

bmjen · 2017-06-06T17:21:26Z

@DavidS @rodjek Discussed this with @scotje yesterday, we also need a mechanism for running bundle update... not sure whether we should do this as a manual operation or programmatic, and if programmatic... at what frequency?

DavidS · 2017-06-06T17:56:20Z

The more we talk about these things, the more I convince myself that iristyle had it right, in that we need to provide a completely version-locked gemset. In that case, folks would never have to bundle update until they want to use the next version of the gemset, where we can hide that in the template update code.

scotje · 2017-06-07T00:17:01Z

Added some acceptance tests. I had to wrangle things quite a bit to be successful invoking pdk inside of a generated module without picking up unwanted context from being in the pdk working directory. I approached it by using a tmpdir and generating a binstub for pdk but the same thing could perhaps be accomplished with BUNDLE_GEMFILE.

Acceptance tests are currently not working in AppVeyor but I've run out of time tonight to debug, perhaps @DavidS or @james-stocks can take a look during their workday.

rodjek · 2017-06-07T05:50:25Z

lib/pdk/util.rb

+    #
+    # @return [String] Fully qualified path to per-user PDK cachedir.
+    def cachedir
+      File.join(Dir.home, '.pdk', 'cache')


It might be cleaner on Windows to have the cache dir in %AppData%

james-stocks · 2017-06-07T10:57:27Z

spec/acceptance/bundle_management_spec.rb

+    end
+
+    # TODO: come up with a way to invoke only the bundler stuff without trying to run unit tests
+    describe command("cd test_module && #{path_to_pdk} test unit") do


This fails on powershell as && is not an operator there.
I think the only command that works on both Linux and PS is "cd test_module; #{path_to_pdk} test unit"

That comes with the weakness of always running command B even if command A fails.
There are workarounds to have Powershell fail and not run command B; but it would mean a command that doesn't work in Linux.

Maybe we could have a helper to hide the platform specifics, like

describe command("cd test_module").chained_to_command.(#{path_to_pdk} test unit") do

?

I've checked serverspec's source, and there doesn't seem to be any way to specify any runtime environment (like CWD) to the command().

james-stocks · 2017-06-07T14:37:36Z

@rodjek @DavidS I added one commit to get appveyor passing; but I think the current failures might be because binstub scripts don't work in Windows?
i.e. I cannot do this on my Windows VM:

C:/projects/pdk/bin/pdk test unit

...Windows just prompts me to pick a program for this unknown filetype.

I'm unsure if this is exactly why appveyor is failing; I'm not seeing the Exec format error errors it's encountering

DavidS · 2017-06-07T15:11:02Z

spec/acceptance/bundle_management_spec.rb

+    @old_pwd = Dir.pwd
+    @tmpdir = Dir.mktmpdir('pdk-acceptance')
+
+    Dir.chdir(@tmpdir)


This will only work locally, but not when using beaker. We'll need to extend beaker-testmode_switcher's API to provide better support for such operations.

Oh yes, I originally had the tmpdir code in spec_helper_acceptance and wrapped in a conditional for testmode == local. It would definitely be better to have the functionality built into the tooling.

DavidS · 2017-06-07T16:33:49Z

@james-stocks these errors are shell_ex from beaker-tms, not serverspec's command(), I'm pretty sure the two use different ways to execute commands, I would not be surprised if a single path_to_pdk does not work for them at all

scotje · 2017-06-07T22:51:30Z

Going to split the acceptance tests back out of this PR, there is a distinct ticket for them anyway and it seems like there are some Windows things that need to be worked out.

scotje · 2017-06-07T23:04:07Z

Made the cachedir changes suggested by @rodjek and squashed.

scotje · 2017-06-07T23:04:53Z

lib/pdk/cli/exec.rb

@@ -46,9 +50,24 @@ def self.git_bindir

      def self.git(*args)
        vendored_bin_path = File.join(git_bindir, 'git')
-        git_path = File.exists?(vendored_bin_path) ? vendored_bin_path : 'git'
-        PDK.logger.debug(_("Using git from the system PATH, instead of '%{vendored_bin_path}'") % { vendored_bin_path: vendored_bin_path})


note that this was always logging this message even when the vendored bin was being used :)

scotje force-pushed the 261_bundle_install branch 5 times, most recently from e4dcc9c to 3187d16 Compare June 5, 2017 22:26

rodjek suggested changes Jun 6, 2017

View reviewed changes

DavidS suggested changes Jun 6, 2017

View reviewed changes

scotje force-pushed the 261_bundle_install branch from 3187d16 to abcd7ff Compare June 6, 2017 21:10

rodjek reviewed Jun 7, 2017

View reviewed changes

james-stocks reviewed Jun 7, 2017

View reviewed changes

DavidS reviewed Jun 7, 2017

View reviewed changes

(SDK-261) Manage basic bundler operations for module dev

cf0169f

scotje force-pushed the 261_bundle_install branch from 1bd1840 to cf0169f Compare June 7, 2017 23:03

scotje commented Jun 7, 2017

View reviewed changes

bmjen merged commit 34dc69a into puppetlabs:master Jun 7, 2017

scotje mentioned this pull request Jun 7, 2017

(SDK-269) Add acceptance tests for bundle management. #68

Merged

scotje deleted the 261_bundle_install branch June 8, 2017 00:18

DavidS added the feature label Jun 16, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(SDK-261) Manage basic bundler operations for module dev #62

(SDK-261) Manage basic bundler operations for module dev #62

scotje commented Jun 5, 2017

rodjek Jun 6, 2017

scotje Jun 6, 2017

rodjek Jun 7, 2017

DavidS left a comment

DavidS Jun 6, 2017

DavidS Jun 6, 2017

scotje Jun 6, 2017

DavidS Jun 6, 2017

scotje Jun 6, 2017

DavidS Jun 6, 2017

scotje Jun 6, 2017

DavidS Jun 6, 2017

scotje Jun 6, 2017

DavidS Jun 6, 2017

scotje Jun 6, 2017

DavidS Jun 6, 2017

DavidS commented Jun 6, 2017

scotje commented Jun 6, 2017

bmjen commented Jun 6, 2017

DavidS commented Jun 6, 2017

scotje commented Jun 7, 2017

rodjek Jun 7, 2017

scotje Jun 7, 2017

james-stocks Jun 7, 2017 •

edited

Loading

DavidS Jun 7, 2017

james-stocks commented Jun 7, 2017

DavidS Jun 7, 2017

scotje Jun 7, 2017

DavidS commented Jun 7, 2017

scotje commented Jun 7, 2017

scotje commented Jun 7, 2017

scotje Jun 7, 2017

DavidS Jun 8, 2017

(SDK-261) Manage basic bundler operations for module dev #62

(SDK-261) Manage basic bundler operations for module dev #62

Conversation

scotje commented Jun 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavidS 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

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

DavidS commented Jun 6, 2017

scotje commented Jun 6, 2017

bmjen commented Jun 6, 2017

DavidS commented Jun 6, 2017

scotje commented Jun 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

james-stocks Jun 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

james-stocks commented Jun 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavidS commented Jun 7, 2017

scotje commented Jun 7, 2017

scotje commented Jun 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

james-stocks Jun 7, 2017 •

edited

Loading