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.

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

Error message when state with multiple destinations is missing a default option #50

Open
PeterJCLaw opened this issue Apr 18, 2018 · 2 comments

Comments

@PeterJCLaw
Copy link
Contributor

PeterJCLaw commented Apr 18, 2018

If you have a config which looks like:

state_machines:
  the_workflow:
    states:
      - gate: choice
        triggers:
          - event: entry
        exit_condition: true
        next:
          type: context
          path: metadata.has_option_a
          destinations:
            - value: false
              state: option_a
            - value: true
              state: option_b

      - gate: option_a
        exit_condition: false

      - gate: option_b
        exit_condition: false

Then when loading the config outputs an exception:

  File ".../routemaster/routemaster/config/loader.py", line 198, in _load_gate
    next_states=_load_next_states(path + ['next'], yaml_state.get('next')),
  File ".../routemaster/routemaster/config/loader.py", line 284, in _load_next_states
    return _load_context_next_states(path, yaml_next_states)
  File ".../routemaster/routemaster/config/loader.py", line 312, in _load_context_next_states
    default=yaml_next_states['default'],
KeyError: 'default'

This suggests that we're requiring that a state's next mapping contains a 'default' key when the next has a destinations, yet that doesn't match that schema.

I'm not sure whether this is a bug in the schema or is something which we can't express in the schema and should output a better error message for.

When fixing this we should also ensure that we check suitably for the reverse case (a default key without a destinations key). I don't know what the right behaviour is in such cases, though I suspect it's not currently being enforced.

PeterJCLaw added a commit that referenced this issue Apr 18, 2018
Otherwise we get an error, though it's not clear if this is expected
(see #50).
@danpalmer
Copy link
Contributor

Yep, I think I just missed this in the schema, so we should add that. I think all destinations should have a default so that developers have to think about what should happen if the data doesn't match what they expected, which it can do given that it's unstructured.

As for the reverse case, I don't think we should support a default with no destinations, because that's just a less clear expression of the shorthand we already have:

- gate: foo
  exit_condition: true
  next: bar

@danpalmer
Copy link
Contributor

danpalmer commented Apr 20, 2018

To be clear, we should:

  • Add a test that we get a schema error if no default is provided.
  • Fix that test by updating the schema.
  • Ensure that the schema requires a non-empty destinations list if not using the shorthand, and add a test if there isn't already something exercising this case.

I think this should be relatively straightforward, so I've added the "easy" label, it should be reasonable for a first time contributor such as @mthpower...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants