Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[WIP] Abort if StartAt or Next [state] is not valid #125

Closed
wants to merge 1 commit into from
Closed

[WIP] Abort if StartAt or Next [state] is not valid #125

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions lib/floe/workflow.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,8 @@ def run_nonblock

def step_nonblock
return Errno::EPERM if end?
return 0 unless step_next

step_next
current_state.run_nonblock!
end

Expand Down Expand Up @@ -103,7 +103,21 @@ def current_state
private

def step_next
context.state = {"Name" => context.next_state, "Input" => context.output} if context.next_state
desired_state = context.next_state || context.state_name
if !@states_by_name.key?(desired_state)
context.state["Error"] = "States.Runtime"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this actually a runtime error? It seems like more of a workflow definition error.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We had a task/issue somewhere to validate workflows before we run them and raise exceptions early. I'm wondering if now is the time to add in that logic, and then have a separate Workflow#validate or Workflow#valid? method with this as the first thing we check for. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WIP: make this a payload parsing check

That's maybe similar to what I'm saying, but it's not a parse failure and more of a logical validation failure. I think of parsing failures more of like being bad syntax. Perhaps I'm over-engineering though?

context.state["Cause"] = "State \"#{desired_state}\" does not exist"
context.output = {"Error" => context.state["Error"], "Cause" => context.state["Cause"]}
context.execution["StartTime"] ||= Time.now.utc
context.execution["EndTime"] = Time.now.utc.iso8601
return false
end

if context.next_state
context.state = {"Name" => context.next_state, "Input" => context.output}
end

true
end
end
end
20 changes: 20 additions & 0 deletions spec/workflow_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,26 @@
expect(ctx.running?).to eq(true)
expect(ctx.ended?).to eq(false)
end

it "detects invalid StartAt" do
workflow = Floe::Workflow.new({"StartAt" => "Bogus", "States" => {"FirstState" => {"Type" => "Succeed"}}}, ctx, {})

workflow.run_nonblock

expect(ctx.running?).to eq(false)
expect(ctx.ended?).to eq(true)
expect(ctx.status).to eq("failure")
end

it "detects invalid Next" do
workflow = Floe::Workflow.new({"StartAt" => "FirstState", "States" => {"FirstState" => {"Type" => "Pass", "Next" => "Bogus"}}}, ctx, {})

workflow.run_nonblock

expect(ctx.running?).to eq(false)
expect(ctx.ended?).to eq(true)
expect(ctx.status).to eq("failure")
end
end

describe "#step" do
Expand Down