adomokos / light-service

Series of Actions with an emphasis on simplicity.
MIT License
837 stars 67 forks source link

Add tests for skipping inside `reduce_if` #164

Closed ramontayag closed 5 years ago

ramontayag commented 5 years ago

@adomokos what would you say is the correct behaviour here? Should it stop the rest of the actions?

adomokos commented 5 years ago

If I recall correctly, we wanted to scope the skip_remaining! within the actual code block and this was the intended behavior. I couldn't find a supporting spec to confirm this. :-(

I would suggest an alternate skip_remaining!, like halt! or something to stop processing immediately without a failure from any level of the orchestrator.

ramontayag commented 5 years ago

I like that. Makes sense to have two versions. I can rename this spec so it reflects what you just mentioned.

adomokos commented 5 years ago

Let's change this test to cover the current functionality, so it passes. I'd clone this test for the new method: halt!. FTR: fail! should stop everything immediately.

ramontayag commented 5 years ago

I forgot that rubocap was failing! @adomokos this passes now except for some Travis build issue.

adomokos commented 5 years ago

It seems the build is still failing. Can you plz take a look?

ramontayag commented 5 years ago

Ok done @adomokos

adomokos commented 5 years ago

Thank you! 👍

and-enaruta commented 1 year ago

@adomokos What is the reason to scope skip_remaining! in reduce_if? I found it a little bit misleading especially when the documentation says "...behaves similarly to fail!, except this will not push the context into a failure state".

ramontayag commented 1 year ago

@and-enaruta this is if you want to skip just the actions in the reduce_if, and not the whole thing.

[
  RunsOne,
  RunsTwo,
  reduce_if(true, [
    RunsThree,
    RunsFour, # skip_remaining called here
    DoesNotRun
  ])
  RunsFive,
]
adomokos commented 1 year ago

Iirc, skip_remaining! works outside of reduce_if as well.

For example:

[
  RunsOne,
  RunsTwo,
  RunsThree,
  RunsFour, # skip_remaining called here
  RunsFive,
]

RunsFive will not be invoked (it is skipped), and the context is still in a success state.

and-enaruta commented 1 year ago

@adomokos @ramontayag Thank you for your quick response. The question is why skip_remaining! need to be scoped by reduce_if?

Here is the business case that I ran into:

[
  RunsOne,
  RunsTwo,
  reduce_if(feature_flag_on, [
    RunsThree,
    RunsFour, # skip_remaining called here
  ]),
  RunsFive,
]

We hide the code behind feature flags. In the above implementation RunsFive will always run. When the feature flag will be removed the same code will work differently and start skipping RunsFive.

For now, as a quick workaround, I did following:

[
  RunsOne,
  RunsTwo,
  reduce_if_else(feature_flag_on, [
    RunsThree,
    RunsFour, # skip_remaining called here
    RunsFive,
  ], [
    RunsFive,
  ]),
]

But it is not scalable when you have multiple feature flags for multiple actions.

adomokos commented 1 year ago

Oh, I see, this might be a scoping issue.