-
Notifications
You must be signed in to change notification settings - Fork 170
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
Make oauth tokens TTL configurable #448
Conversation
10642f3
to
a0b0214
Compare
Spell Checker found issuesdoc/parameters.md
Generated by 🚫 Danger |
t/005-apicast-oauth.t
Outdated
[error] | ||
|
||
=== TEST 18: default token TTL | ||
When a default TTL is not specified, it applies a default of 7 days (604800 s) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you try with empty value? That is common OpenShift fuckup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I added a new test (TEST 19
) which is basically a copy-pasta of TEST 18
. The only difference is an env. Any ideas to avoid this kind of duplication in Test::Nginx ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No idea :( It really sucks to have this kind of duplication.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe would be useful to test it on lower level - in busted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. I thought about that, but then I saw that there are no unit tests for oauth, so I thought that it would be better to just complete the existing integration tests for now. Maybe this is something we could address in a future PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely. 👍
a0b0214
to
a9338d4
Compare
Fixes #190