ManageIQ / floe

Floe is a runner for Amazon States Language workflows
Apache License 2.0
0 stars 5 forks source link

Pass ruby objects between states #222

Closed kbrock closed 2 months ago

kbrock commented 2 months ago

Overview

See also #228

Built upon:

I am displaying these opinions so we can more easily discus them:


kbrock commented 2 months ago

update:

agrare commented 2 months ago

@kbrock I think we could use some specs covering using simple strings not hashes as input/output

I applied this to https://github.com/ManageIQ/floe/pull/184 and I'm seeing

  1) Floe::Workflow::States::Map#run_nonblock! has no next
     Failure/Error: output&.key?("Error") || false

     NoMethodError:
       undefined method `key?' for "R31":String

               output&.key?("Error") || false
                     ^^^^^^
     # ./lib/floe/workflow/context.rb:39:in `failed?'
     # ./lib/floe/workflow/state.rb:67:in `finish'
     # ./lib/floe/workflow/states/non_terminal_mixin.rb:11:in `finish'
     # ./lib/floe/workflow/states/pass.rb:30:in `finish'
     # ./lib/floe/workflow/state.rb:51:in `run_nonblock!'
     # ./lib/floe/workflow_base.rb:32:in `step_nonblock'
     # ./lib/floe/workflow_base.rb:25:in `run_nonblock'
     # ./lib/floe/workflow/states/map.rb:90:in `step'
     # ./lib/floe/workflow/states/map.rb:57:in `start'
     # ./lib/floe/workflow/state.rb:48:in `run_nonblock!'
     # ./spec/workflow/states/map_spec.rb:110:in `block (3 levels) in <top (required)>'

This is fixed by my changes https://github.com/ManageIQ/floe/pull/214/files#diff-c61a9c0cebe305116e077d04dc168aa33f6339d348c778addf65ab7c19b2479bR42 and https://github.com/ManageIQ/floe/pull/214/files#diff-24e71eb9cd8cfb4211a041b7a08cfc1d9d33203a6c2ce88dbc153234e82dfe1cR67

kbrock commented 2 months ago

@agrare thanks for the pointer on this one I was getting the same errors I merged 184 and they seem to be resolved

Fryguy commented 2 months ago

Had a question here about the intent. I noticed a comment in #214 where we are expecting "non-json" output, but to clarify, we really are expecting "non-hash" output. Well-formed JSON strings or integers are still JSON even if they aren't a Hash. So my concern here if we don't attempt to JSON parse it, is that invalid outputs can be emitted and we would miss verifying them.

So, here we see valid JSON "bare" strings and integers:

[1] pry(main)> require "json"
=> true
[2] pry(main)> "test".to_json
=> "\"test\""
[3] pry(main)> JSON.parse(_)
=> "test"
[4] pry(main)> 1.to_json
=> "1"
[5] pry(main)> JSON.parse(_)
=> 1

But this would be invalid output/input:

[2] pry(main)> JSON.parse("foo")
~/.gem/ruby/3.1.5/gems/json-2.7.2/lib/json/common.rb:220:in `parse': unexpected token at 'foo' (JSON::ParserError)
    from ~/.gem/ruby/3.1.5/gems/json-2.7.2/lib/json/common.rb:220:in `parse'
    from (pry):2:in `__pry__'
[3] pry(main)> JSON.parse('{\"xxx":')
~/.gem/ruby/3.1.5/gems/json-2.7.2/lib/json/common.rb:220:in `parse': unexpected token at '{\"xxx":' (JSON::ParserError)
    from ~/.gem/ruby/3.1.5/gems/json-2.7.2/lib/json/common.rb:220:in `parse'
    from (pry):3:in `__pry__'
kbrock commented 2 months ago

update:

outstanding:

kbrock commented 2 months ago

update:

kbrock commented 2 months ago

un-wip: we are now outputting in json (thanks to other PR)

miq-bot commented 2 months ago

Checked commits https://github.com/kbrock/floe/compare/9262ef55d95376a6484d50fd80086af322052c14~...3dab1bae003c76fe1d26a719afa5f2a33c16055d with ruby 3.1.5, rubocop 1.56.3, haml-lint 0.51.0, and yamllint 5 files checked, 0 offenses detected Everything looks fine. :+1:

miq-bot commented 2 months ago

This pull request is not mergeable. Please rebase and repush.

kbrock commented 2 months ago

We've decided to do json in the context.