Hockenba commented on PR #13651:
URL: https://github.com/apache/apisix/pull/13651#issuecomment-5434273687
> 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
You make a lot of great points. It is a little late in my time zone, so I'll
refine this tomorrow based on your comments.
One thing: the Chinese documentation needs to the created and pushed to this
PR by someone who actually knowns Chinese. I do not know a lick of Chinese and
I am sure we want better than what Google Translate will spit out for me.
--
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]
