(BOLT-286) Convert ExecutionResult to ResultSet by adreyer · Pull Request #253 · puppetlabs/bolt

adreyer · 2018-01-19T23:09:57Z

This standardizes how results are handled in bolt and puppet. Previously
a hash Bolt::Result objects were generated by they executor when a
run_* action was taken from puppet. The puppet run_* functions then
convverted those into and ExecutionResult. Now Results are built into a
Bolt::ResultSet object in the executor. Both this and the bolt result
are now pcore datatypes and available directly in the plan.

MikaelSmith · 2018-01-19T23:43:04Z

        end

+        # Now that puppet is loaded we can include puppet mixins in data types
+        Bolt::ResultSet.include_iterable


How is Bolt::ResultSet being included? Does it matter that we won't be able to mixin iterable for commands/scripts?

I guess the 1st question is answered: bolt/executor.

Ah, this is preserving previous behavior. There's fallback for iterable when not using Puppet.

I don't actually know when the fallback gets used. the ruby mixin Enumerable uses each and powers most of the iterator like behavior in ruby code(either in puppet functions or bolt)

MikaelSmith · 2018-01-19T23:46:45Z

-          if result.instance_of? Bolt::ExecutionResult
-            result = result.unwrap
-          end
+          # TODO: Is there anything we still need to unwrap?


Do Puppet::DataTypes::Error display the way we want them to?

We should be displaying the error_hash not the error datatype in bolt. I may not have fixed that properly

I guess this is an arbitrary data type. If it's a ResultSet, that has a to_s method that writes an array of status_hash

MikaelSmith · 2018-01-19T23:49:47Z

      @notifier.shutdown

-      results_to_hash(results)
+      Bolt::ResultSet.new( results.map { |r| r } )


Why can't we pass results directly? Is there a reason we don't want the concurrent version of an array?

I'm not sure. it was doing this before with the hash.

MikaelSmith · 2018-01-19T23:50:31Z

-      if state == 'finished'
-        exit_code = 0
+      # If it's finished or already has a proper error simply pass it to the
+      # the result  otherwise make sure an error is generated


Two spaces, looks weird.

MikaelSmith · 2018-01-19T23:56:09Z

-          output.exit_code)
+    # TODO: what to call this it's the value minus special keys
+    # This should be {} if a value was set otherwise it's nil
+    def generic_value


We could call this value, and use something more generic (data?) for the pieces including special keys. I'm ok with generic_value though.

MikaelSmith · 2018-01-20T00:15:12Z

+  end
+
+  # TODO: should this exist?
+  def [](node_uri)


Asking whether you should be able to look a value up by its name/uri? I feel like the one case that could be useful is when cross-referencing between several results. Like: several nodes performed an action against load-balanced servers, I want to group the results of that action by the server they operated against.

I feel like I'm having to stretch for examples, but it doesn't feel like much work to enable some use-cases we can't envision right now.

I guess find already allows that. Are you suggesting that as better than []?

find seems clearer. I'm not sure how [] should behave or if we should expose it when the underlying data structure is an array.

MikaelSmith · 2018-01-20T00:17:15Z

+    self[node_uri]
+  end
+
+  # TODO delete me?


Anything we don't use and that's not exposed in the Puppet DataType seems like we can remove.

MikaelSmith · 2018-01-20T00:18:12Z

+    self.class == other.class && @results == other.results
+  end
+
+  # TODO: this loses target information


Target data seems like something we'd want when serializing, but I'm not sure what we're using the serialization for.

MikaelSmith · 2018-01-20T00:18:46Z

    end

+    # name is currently just uri but should be be used instead to identify the
+    # Target ouside the transport or uri options.


That seems like hostname...

The point of name is to give us/the user flexibility to define what matters for identity. hostname suffers from the same problem as URI.

For example the user to connect as may be part of the identity if you are running different commands as different users but it also could just be connection information. I suspect this will be a case where identity with default to url but users may end up overriding it eventually as needed.

MikaelSmith · 2018-01-20T00:20:04Z

  end

  # this is used from 'bolt task run'
  dispatch :run_task_raw do


What is run_task_raw used for? Not sure what we do with the block.

we pass a block to output events when running tasks outside a plan

puppetcla · 2018-01-20T16:00:18Z

CLA signed by all contributors.

MikaelSmith

Generally seems ok. I'll work on some exploratory testing around Result and ResultSet.

adreyer · 2018-01-22T19:45:14Z

-    private
+    # Warning: This will fail outside of a compilation.
+    # Use error_hash inside bolt.
+    # Is it crazy for this to behave differently outside a compiler?


I'm not sure what to do here. I think if we're going to change this behavior it needs to happen in a different ticket

MikaelSmith · 2018-01-22T20:19:42Z

+      self.class == other.class && @results == other.results
+    end
+
+    def to_json


Getting an error

Plan aborted: run_task 'upgrade_pe::monolithic' failed on 1 nodes bundler: failed to load command: bolt (/Users/michaelsmith/puppetlabs/orchestration-control/.bundle/gems/ruby/2.3.0/bin/bolt) ArgumentError: wrong number of arguments (given 1, expected 0) /Users/michaelsmith/puppetlabs/orchestration-control/.bundle/gems/ruby/2.3.0/bundler/gems/bolt-f260b52f88e9/lib/bolt/result_set.rb:82:in `to_json' /Users/michaelsmith/.rbenv/versions/2.3.4/lib/ruby/2.3.0/json/common.rb:286:in `generate' /Users/michaelsmith/.rbenv/versions/2.3.4/lib/ruby/2.3.0/json/common.rb:286:in `pretty_generate' /Users/michaelsmith/puppetlabs/orchestration-control/.bundle/gems/ruby/2.3.0/bundler/gems/bolt-f260b52f88e9/lib/bolt/outputter/human.rb:142:in `fatal_error'

Plan I'm trying to run is

plan upgrade_pe::mono_cms( String[1] $mono_master, String[1] $build, Optional[Variant[String[1], Array[String[1]]]] $compile_masters = undef, ) { $mono = run_task('upgrade_pe::monolithic', $mono_master, build => $build) $mono.each |$result| { if $result.ok { notice("${result.target} completed:\n${result.message}") } else { notice("${result.target} errored with message ${result.error}") } } if ($mono.ok and $compile_masters) { $_compile_masters = $compile_masters.split(',') $cms = run_task('upgrade_pe::compile_master', $_compile_masters, master => $mono_master) $cms.each |$result| { if $result.ok { notice("${result.target} completed:\n${result.message}") } else { notice("${result.target} errored with message ${result.error}") } } } }

this was caused by to_json not accepting opts. it should be fixed now

This standardizes how results are handled in bolt and puppet. Previously a hash Bolt::Result objects were generated by they executor when a run_* action was taken from puppet. The puppet run_* functions then convverted those into and ExecutionResult. Now Results are built into a Bolt::ResultSet object in the executor. Both this and the bolt result are now pcore datatypes and available directly in the plan. This adds the Bolt::PAL class which is only used for testing so far. In the near future all interaction with Puppet should go through this class.

MikaelSmith · 2018-01-22T21:54:05Z

+require 'bolt_spec/pal'
+
+require 'bolt/pal'
+# TODO: clean this up


What's this TODO?

the requires. This should get cleaned up with the PAL work

adreyer added the Work in Progress label Jan 19, 2018

MikaelSmith mentioned this pull request Jan 19, 2018

(maint) Refactor logic out of Node class #255

Merged

MikaelSmith reviewed Jan 20, 2018

View reviewed changes

MikaelSmith reviewed Jan 22, 2018

View reviewed changes

adreyer force-pushed the BOLT-286 branch 3 times, most recently from 6e98039 to f260b52 Compare January 22, 2018 20:09

adreyer commented Jan 22, 2018

View reviewed changes

MikaelSmith reviewed Jan 22, 2018

View reviewed changes

MikaelSmith approved these changes Jan 22, 2018

View reviewed changes

adreyer force-pushed the BOLT-286 branch from e33f993 to f6409dd Compare January 22, 2018 21:25

adreyer force-pushed the BOLT-286 branch from f6409dd to 0ac7e85 Compare January 22, 2018 21:37

MikaelSmith reviewed Jan 22, 2018

View reviewed changes

MikaelSmith merged commit 73f25d6 into puppetlabs:master Jan 22, 2018

Conversation

adreyer commented Jan 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MikaelSmith Jan 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

puppetcla commented Jan 20, 2018

Uh oh!

MikaelSmith left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

MikaelSmith Jan 22, 2018 •

edited

Loading