(SDK-144) Add option to run validate in parallel #144

austb · 2017-07-11T20:55:49Z

This addes the --parallel flag to the pdk validate command. This
flag will cause the validations to run in their own threads.

UPDATE: waiting for release of tty-spinner gem (also probably need to wait for a release of tty-prompt with compatible tty-cursor range.

Remaining

Remove short_spinner_text
Use PDK::Util.spinner_opts_for_platform
Use released versions of tty-spinner and tty-prompt gems
Add Docs

bmjen · 2017-07-12T23:06:13Z

lib/pdk/cli/validate.rb

@@ -15,6 +15,7 @@ module PDK::CLI

    flag nil, :list, _('list all available validators')
    flag :a, 'auto-correct', _('automatically correct problems (where possible)')
+    flag nil, :parallel, _('run validations in parallel')


For the upcoming community release, we'll need to comment this line and hardcode line 74 with false.

Not necessary to comment it out, can just make it a hidden flag (like --answer-file), so that it's still available for dev & testing but not shown in the help output.

rodjek

A couple of refactors, and a behaviour change I'm not sure if we want.

Also, it looks like the de po file has been changed with some bad unicode substitutions (ü => Ÿ etc)?

rodjek · 2017-07-13T04:23:01Z

lib/pdk/cli/validate.rb

+      mod_root = PDK::Util.module_root
+      unless mod_root
+        raise PDK::CLI::FatalError, _('Current working directory is not part of a module. (No metadata.json was found.)')
+      end


This block can be replaced with PDK::CLI::Util.ensure_in_module! however i'm not sure that we can to restrict validators to run in a module (for example, a module consumer would probably want to run pdk validate from the root of their control repo to run validation on everything at once).

I did not view this as a behavior change. previously we had this code around the console command call in cli/exec.rb. This had to be brought out to this level because Dir.chdir changes the working dir of the process, not the thread.

rodjek · 2017-07-13T04:28:02Z

lib/pdk/validators/metadata/metadata_syntax.rb

@@ -14,13 +14,29 @@ def self.pattern
        'metadata.json'
      end

+      def self.spinner_text(targets = [])
+        _('Checking metadata syntax (%{targets})') % {
+          targets: targets.map { |t| Pathname.new(t).absolute? ? Pathname.new(t).relative_path_from(Pathname.pwd) : t }.join(' '),


As this is now being used in multiple places, we should probably split it out into a helper method (PDK::Util.targets_relative_to_pwd or something) to avoid duplicating the logic.

austb · 2017-07-13T21:23:10Z

@rodjek The de po file has been removed entirely from this build. It contained outdated strings which were causing problems when creating new threads. I did not see any way to update a po file while keeping the old strings.

Lots has changed in the PDK and in the parallel validation, so it could use another look over before we merge.

DavidS · 2017-07-24T13:58:29Z

lib/pdk/cli/spinner.rb

+
+    # This is a placeholder until we integrate ansicon into Windows packaging
+    # or come up with some other progress indicator for Windows.
+    class WindowsSpinner


ansicon is already integrated. we can drop this.

We also need unicode before our ✔️ or ✖️ will show up in windows. We could default to the text passed or failed on windows, ie

[passed] Checking metadata (metadata.json)

Would we rather have that so we can have the spinner on Windows? And do we want to do that in this PR?

having the words instead of the unicode chars is a good idea. I wouldn't block the PR for the change, but we need to do it at one point....

Ok, I'll leave it as is for now, but get a fix for that up once we get this through.

DavidS · 2017-07-24T13:59:04Z

lib/pdk/cli/spinner.rb

+  module CLI
+    module Spinner
+      PREFIX = 'spinner_key'.freeze
+      LOCK = Monitor.new


locks should be private to avoid other callers violating expectations.

DavidS · 2017-07-24T14:00:50Z

lib/pdk/cli/validate.rb

+        threads.each(&:join)
+        exit_codes.sort!
+
+        if exit_codes.last != 0


exit_codes.all? { |c| c == 0 }

https://ruby-doc.org/core-2.4.1/Enumerable.html#method-i-all-3F

I sorted them to return the highest error code with last in order to mirror the functionality in base_validator.rb

So that also needs fixing then: http://ruby-doc.org/core-2.4.0/Enumerable.html#method-i-max

DavidS · 2017-07-24T14:06:31Z

lib/pdk/util.rb

+
+    # Returns the targets' paths relative to the working directory
+    #
+    # @ return Array<String> The absolute or path to the target


should that be @return instead of @ return ?

Where/how can we check the YARD output?

We need to add a rake task to generate YARD output https://github.com/lsegal/yard#generating-documentation

I'll fix the typo and we can have another PR for that if we want to add it.

This is fixed, just not shown as outdated for some reason

DavidS · 2017-07-24T14:07:42Z

locales/de/pdk.po

@@ -1,604 +0,0 @@
-# German translations for puppet development kit package.


this file should not be deleted

This file caused a lot of problems for threads because it was no longer up to date with the current pdk.pot file, and when talking to the i18n folks they were confused why we even had a PO file in our repo as they will be managed by transifex. We should discuss this in more detail if you have concerns, but if we would like both the German PO file and threading the German PO file must be kept up to date and in sync with the pdk.pot file.

An important note, keeping a PO file in sync with the POT file without deleting all the existing translations is not a feature provided by the gettext-setup-gem we use because the Transifex workflow creates brand new PO files and populates them with the previous translations that match current strings. So we would be responsible for figuring out how to do that (most likely using msgmerge).

Another potential solution would be for you to keep the German file locally and not checked in to the repo.

I would be interested how the non-up-to-date de.po file causes issues with threading, especially outside the de locale.

rake gettext:update_pot and rake gettext:po[de] worked fine until now to update the po file as needed, and to the right thing for gettext. I'm sure transifex can one-time import the existing translations once we get around to integrating.

Another potential solution would be for you to keep the German file locally and not checked in to the repo.

I'm not even using the thing, what value would it have on my local machine?

I believe we ran into two error messages, one involves fuzzy messages and the other is for old messages it leaves in comments at the bottom.

Commented

Warning: obsolete msgid exists. #~ msgstr "Verwende ein eigenes Vorlagerepository für dieses Modul."

Fuzzy

Warning: fuzzy message was used. : msgid 'Unable to write '%{file}': %{msg}'

I'm sure transifex can one-time import the existing translations once we get around to integrating.

In my interactions with i18n for this problem, I don't think they are willing to ship a translation that wasn't done by Transifex or whichever company does the translations through Transifex.

I have added a Rake task that cleans the file for us, one extra step to make sure it works, but we can keep the German file now

Excellent work, @austb . Thank you very much for your attention here!

(specifically to discovering the root cause of the uninitialized threads)

DavidS · 2017-07-24T14:08:26Z

I've had a few comments/change requests on first reading.

scotje

Some relatively minor things before we merge today. Austin and I talked about a larger refactor that we try to do before 1.0 release.

scotje · 2017-08-07T20:17:29Z

lib/pdk/cli/exec.rb

@@ -105,7 +104,13 @@ def add_spinner(message, opts = {})
          @success_message = opts.delete(:success)
          @failure_message = opts.delete(:failure)

-          @spinner = Gem.win_platform? ? WindowsSpinner.new(message, opts) : TTY::Spinner.new("[:spinner] #{message}", opts)
+          PDK::Util.spinner_opts_for_platform(opts)


This method should return a new hash and you should merge with existing opts rather than mutating arg.

scotje · 2017-08-07T20:18:06Z

lib/pdk/cli/exec.rb

@@ -1,7 +1,6 @@
 require 'bundler'
 require 'childprocess'
 require 'tempfile'
-require 'tty-spinner'


What is the reason to not explicitly require this here anymore?

scotje · 2017-08-07T20:27:31Z

lib/pdk/cli/exec.rb

-            Dir.chdir(mod_root) do
-              ::Bundler.with_clean_env do
-                run_process!
-              end


I don't see a reason to remove all this, the parallel validate can still call the util method to abort early but I feel like removing this is pushing responsibility in the wrong direction.

We can test Dir.pwd so that we only try to chdir if we aren't already in the right place. That should keep the child threads from complaining.

scotje · 2017-08-07T21:44:47Z

lib/pdk/validators/metadata/metadata_syntax.rb

        targets = parse_targets(options)

        return 0 if targets.empty?

-        return_val = 0
+        @return_val = 0


Was there a reason this had to become an instance var? Something to do with threads?

DavidS

Please do not merge this currently, as it is not passing the internal pipeline for the packaging.

coveralls · 2017-08-08T19:38:08Z

Coverage increased (+0.008%) to 92.238% when pulling 15083b6 on austb:parallel_validation into 47fb4ee on puppetlabs:master.

temp hold pre-release

coveralls · 2017-08-09T17:29:01Z

Coverage increased (+0.008%) to 92.238% when pulling 49ccaa5 on austb:parallel_validation into 47fb4ee on puppetlabs:master.

coveralls · 2017-08-09T17:46:50Z

Coverage increased (+0.008%) to 92.238% when pulling 5727304 on austb:parallel_validation into f8cb71d on puppetlabs:master.

coveralls · 2017-08-09T23:50:20Z

Coverage increased (+0.008%) to 92.238% when pulling cbe341b on austb:parallel_validation into f8cb71d on puppetlabs:master.

scotje · 2017-08-10T18:34:16Z

@jbondpdx Austin is going to add a blurb to the README about this feature and then we plan to merge. Despite the size of the PR, the actual user impact/README stuff should be pretty simple.

This adds the `--parallel` flag to the `pdk validate` command. This flag will cause the validations to run in their own threads.

coveralls · 2017-08-10T19:06:42Z

Coverage increased (+0.008%) to 92.248% when pulling 6e45cd2 on austb:parallel_validation into 3a7e3d5 on puppetlabs:master.

austb force-pushed the parallel_validation branch 4 times, most recently from b5501f0 to 502fd09 Compare July 12, 2017 21:05

bmjen reviewed Jul 12, 2017

View reviewed changes

austb force-pushed the parallel_validation branch 3 times, most recently from e77123e to dcef710 Compare July 12, 2017 23:53

rodjek suggested changes Jul 13, 2017

View reviewed changes

bmjen added this to the 0.5.0 milestone Jul 13, 2017

bmjen added the feature label Jul 13, 2017

rodjek previously approved these changes Jul 14, 2017

View reviewed changes

austb force-pushed the parallel_validation branch 9 times, most recently from 14aea40 to c385e98 Compare July 20, 2017 18:27

austb force-pushed the parallel_validation branch from c385e98 to 5e4ef24 Compare July 20, 2017 18:51

DavidS reviewed Jul 24, 2017

View reviewed changes

scotje suggested changes Aug 7, 2017

View reviewed changes

scotje changed the title ~~{WIP} (SDK-144) Add option to run validate in parallel~~ (SDK-144) Add option to run validate in parallel Aug 7, 2017

scotje approved these changes Aug 7, 2017

View reviewed changes

DavidS previously requested changes Aug 8, 2017

View reviewed changes

austb added the blocked label Aug 8, 2017

austb force-pushed the parallel_validation branch from af21753 to 8570f99 Compare August 8, 2017 19:13

puppetlabs deleted a comment from coveralls Aug 8, 2017

austb removed the blocked label Aug 9, 2017

austb force-pushed the parallel_validation branch from 49ccaa5 to 5727304 Compare August 9, 2017 17:40

austb mentioned this pull request Aug 9, 2017

Update build sources for parallel validation puppetlabs/pdk-vanagon#48

Merged

scotje approved these changes Aug 10, 2017

View reviewed changes

(SDK-144) Add option to run validate in parallel

6e45cd2

This adds the `--parallel` flag to the `pdk validate` command. This flag will cause the validations to run in their own threads.

austb force-pushed the parallel_validation branch from cbe341b to 6e45cd2 Compare August 10, 2017 19:00

austb merged commit 4a68817 into puppetlabs:master Aug 10, 2017

austb deleted the parallel_validation branch August 10, 2017 20:42

		@@ -1,604 +0,0 @@
		# German translations for puppet development kit package.

(SDK-144) Add option to run validate in parallel #144

(SDK-144) Add option to run validate in parallel #144

Conversation

austb commented Jul 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodjek 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

austb commented Jul 13, 2017

Choose a reason for hiding this comment

austb Jul 24, 2017 • edited Loading

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

austb Jul 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

austb Jul 24, 2017 • edited Loading

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

austb Jul 24, 2017 • edited Loading

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 Jul 24, 2017

scotje 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

DavidS left a comment

Choose a reason for hiding this comment

coveralls commented Aug 8, 2017

coveralls commented Aug 9, 2017

coveralls commented Aug 9, 2017

coveralls commented Aug 9, 2017

scotje commented Aug 10, 2017

coveralls commented Aug 10, 2017

austb commented Jul 11, 2017 •

edited

Loading

austb Jul 24, 2017 •

edited

Loading

austb Jul 24, 2017 •

edited

Loading

austb Jul 24, 2017 •

edited

Loading

austb Jul 24, 2017 •

edited

Loading