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

Add simple response #214

Merged
merged 1 commit into from
Jul 4, 2023
Merged

Add simple response #214

merged 1 commit into from
Jul 4, 2023

Conversation

pehala
Copy link
Contributor

@pehala pehala commented Jun 2, 2023

  • Add simple debugging response - response.add_simple
    • Used when you need to return some authjson section and do not care about details
  • Refactor extract_from_response
    • Uses weakget
    • same default values as response.add_simple

@pehala pehala requested review from azgabur and jsmolar June 2, 2023 06:51
@pehala pehala force-pushed the extract branch 2 times, most recently from 900a93d to 4d2b6d9 Compare June 2, 2023 07:13
mganisin
mganisin previously approved these changes Jun 15, 2023
View reviewed changes
@pehala
Add simple response
f5594f6 
- refactor extract_from_response
@azgabur azgabur mentioned this pull request Jul 3, 2023
Closed
@pehala pehala merged commit bb4d78e into Kuadrant:main Jul 4, 2023
azgabur
azgabur reviewed Nov 9, 2023
View reviewed changes
testsuite/tests/kuadrant/authorino/conditions/section_conditions/test_response_condition.py
@@ -31,10 +31,10 @@ def test_skip_response(client, auth):
assert response.status_code == 200

# verify that response was not returned on a GET request
with pytest.raises(KeyError, match="Auth-Json"):
extract_from_response(response)
assert "simple" not in response.json()["headers"]
Copy link
Contributor

@azgabur azgabur Nov 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Found a bug. This string should be Simple because HTTP headers capitalize first letter of header name. As this PR is already merged, can you create a new small one with this fix @pehala ? Opened #272

@azgabur azgabur mentioned this pull request Nov 9, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@azgabur azgabur azgabur approved these changes

@mganisin mganisin mganisin left review comments

@averevki averevki averevki approved these changes

@jsmolar jsmolar Awaiting requested review from jsmolar

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pehala @mganisin @averevki @azgabur