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

Fix wrong credentials param test_auth_credentials.py #263

Merged
merged 1 commit into from
Nov 6, 2023

Conversation

azgabur
Copy link
Contributor

@azgabur azgabur commented Nov 3, 2023

Fix wrong credentials parameters that were missed in #248
It is interesting that it was not caught in automated commit test via mypy tool because the variables are behind pytest parametrize feature.

@azgabur azgabur requested review from pehala and averevki November 3, 2023 13:48
Copy link
Contributor

@averevki averevki left a comment

Choose a reason for hiding this comment

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

Good find! With it, I think you should fix it also in another missed place 🫤:

def test_custom_selector(client, auth, credentials):
"""Test if auth credentials are stored in right place"""
response = client.get("/get", headers={"authorization": "API_KEY " + auth.api_key})
assert response.status_code == 200 if credentials == "authorization_header" else 401

Would be great to also fix the tests there, like the fix from #246 😊

@pehala
Copy link
Contributor

pehala commented Nov 6, 2023

Good find! With it, I think you should fix it also in another missed place 🫤:

def test_custom_selector(client, auth, credentials):
"""Test if auth credentials are stored in right place"""
response = client.get("/get", headers={"authorization": "API_KEY " + auth.api_key})
assert response.status_code == 200 if credentials == "authorization_header" else 401

Would be great to also fix the tests there, like the fix from #246 😊

Could be, but at the same time, it can be done in another PR, feel free to create one :)

@azgabur azgabur force-pushed the fix_fix_auth_credentials branch from 629ea00 to 28d5ef9 Compare November 6, 2023 09:55
@azgabur
Copy link
Contributor Author

azgabur commented Nov 6, 2023

^ renamed commit

@azgabur
Copy link
Contributor Author

azgabur commented Nov 6, 2023

Created the other MR :)
#265

@pehala pehala merged commit 6aafbf0 into Kuadrant:main Nov 6, 2023
2 checks passed
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