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

always return set payload body for error codes #27

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

james-bowers
Copy link
Contributor

@james-bowers james-bowers commented Jul 3, 2020

We don't want to overwrite the provided response body when a recipe returns anything other than a 200.

{406, "/*"} -> {:ok, OriginSimulator.recipe_not_set()}
{406, _} -> {:ok, OriginSimulator.recipe_not_set(path)}
_ -> {:ok, "Error #{status}"}
_ -> cache_lookup(route)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry maybe I'm missing the reasoning for this. I'm not sure why it's trying to fetch from cache for 404s

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah it's probably the discussion in Slack. It'd be useful to add a line in the PR desc

Copy link
Contributor

@boonious boonious left a comment

Choose a reason for hiding this comment

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

This does mean that scenarios such as this one in README, would return 200 origin payload for the specified statuses: https://github.com/bbc/origin_simulator#scenarios .

Copy link
Contributor

@boonious boonious left a comment

Choose a reason for hiding this comment

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

An improvement to be able to set 404/500 body in single-stage recipes (vs. same payload in multi-stages-statues recipes which could be emulated by updating OS recipe during test).

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

Successfully merging this pull request may close these issues.

3 participants