ResultPath=$ replaces complete output #199
Conversation
|
update:
|
lib/floe/workflow/reference_path.rb
Outdated
| return value.dup if path.empty? | ||
|
|
||
| child = result = context.dup | ||
| path[..-2].each do |key| |
There was a problem hiding this comment.
path[..-2] is melting my brain...is it chomping off the last character? If so, is it chomping off a specific character (asking cause then you can use the chomp method with a parameter.
There was a problem hiding this comment.
yea. I wanted to get rid of the dup, but maybe this is the same.
This is
- keys = path.dup
- last_key = keys.pop
- keys.each do |key|
+ path[..-2].each do
...
end
- child[last_key]
+ child[path.last]probably same as:
- keys = path.dup
- last_key = keys.pop
+ *keys, last_key = pathThough all of these may allocate the same so this may be an unnecessary change.
There was a problem hiding this comment.
@kbrock can we tackle this in a different refactoring PR if https://github.com/ManageIQ/floe/pull/199/files#r1622463119 is the primary fix here?
There was a problem hiding this comment.
I don't know if it is any better.
Will probably forget all about this change. (as it should be)
Good call getting in the important stuff and changing as little as possible
lib/floe/workflow/reference_path.rb
Outdated
| if path.empty? | ||
| result.merge!(value) |
There was a problem hiding this comment.
This is the main change
if path.empty?
- result.merge!(value)
+ value.dup
spec/workflow/reference_path_spec.rb
Outdated
| let(:input) { {} } | ||
| let(:input) { { "old" => "key"} } |
There was a problem hiding this comment.
this was an empty hash before, so the tests did not show the distinction here.
Adding a before value caused the code to show the nuance I am highlighting
|
Great catch @kbrock |
spec/workflow/reference_path_spec.rb
Outdated
| @@ -50,19 +50,27 @@ | |||
|
|
|||
| describe "#set" do | |||
| let(:payload) { "$" } | |||
| let(:input) { {} } | |||
| let(:input) { { "old" => "key"} } | |||
There was a problem hiding this comment.
Rubocop has a good point
| let(:input) { { "old" => "key"} } | |
| let(:input) { {"old" => "key"} } |
kbrock
force-pushed
the
empty_path
branch
2 times, most recently
from
4732951
to
b20a993
Compare
|
update:
update:
|
ok, https://states-language.net/#error-output > The default value, if the "ResultPath" field is not provided, is "$", meaning that the output consists entirely of the Error Output. Which differs from the previous code https://github.com/ManageIQ/floe/blob/master/lib/floe/workflow/reference_path.rb#L28-L29 > If the payload is '$' then merge the value into the context This fixes issues where the output is a simple class inserting at the top level
|
Checked commit kbrock@c3ba972 with ruby 2.7.8, rubocop 1.56.3, haml-lint 0.51.0, and yamllint |
Fixed - ResultPath=$ replaces complete output (#199) - Fix retrier backoff values (#200) - Fix Retry issues (#202) - Add Apache-2.0 license (#217) Changed - Update gemspec summary (#205) - Simpler State#long_name (#204) - State only modifies Context#state - prep for Map/Parallel (#206) - Set StateHistory in Workflow not State (#211) - Make Runner#wait optional (#190) - Pass credentials around with context (#203) - Pass context to State without workflow (#216) - Move the guts of the CLI into a class for easy testing (#220) Added - Set State PreviousStateGuid in StateHistory (#208) - Add a codeclimate config file (#224) - Add an Execution unique ID to Context (#226)
ok, https://states-language.net/#error-output
Which differs from the previous code
https://github.com/ManageIQ/floe/blob/master/lib/floe/workflow/reference_path.rb#L28-L29
This fixes issues where the output is a simple class inserting at the top level
I stumbled across this because the
TaskCatchshould output the error as the output node