janiussyafiq commented on PR #13651:
URL: https://github.com/apache/apisix/pull/13651#issuecomment-5434058660
Some comments that you can address or state your reasoning on why you think
the current approaches should remain:
- I think it would be cleaner to expose the schema in this way rather than
the current way since lua-resty-session also do it the same way instead nesting
it within redis, WDYT?
```json
"session": {
"storage": "cookie",
"redis": { "host": "127.0.0.1" },
"revocation": "redis",
"revocation_fail_mode": "closed"
}
```
- Docs comment: can add one section for usage example using this current
feature (can refer how other example showcases this), and add docs for chinese
version too
- Update PR desc to reflect the newest code submitted for review, some of
them were stale
- IMO the tests were bloated and some are mere duplication from
`lua-resty-session` upstream test which have already been covered, might
consider rechecking again. e.g.
- TEST 13-16 exercise lua-resty-session's fail-open/fail-closed behaviour
through a stub `revocation` table and the library's private `__set_ngx_header`
/ `__set_ngx_var` hooks. That path is already covered upstream
(`spec/07-revocation-2_spec.lua`); APISIX's contribution is one field, which
TEST 2 already asserts.
- TEST 13-17 accept `skip: revocation not supported` in `response_body`,
so they pass without testing anything on lua-resty-session < 4.2.0. With the
rockspec pinned to 4.2.0-1 the probe should go; a missing feature should fail,
not skip (same reason `check-test-code-style.sh` bans `--- SKIP`).
- The 12 schema/passthrough blocks differ by one or two fields each.
`t/plugin/openid-connect2.t` (TEST 1) folds such variants into a `test_cases`
table in one block; the same would bring this file down to a handful of blocks.
- Four cases from the test plan have no test: default
`revocation_fail_mode` (every block sets it explicitly), `mode=storage` on
cookie, `revocation_fail_mode` without `redis`, invalid `redis.mode`.
- etc. You might want to revisit this again
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
