rusackas commented on PR #38092:
URL: https://github.com/apache/superset/pull/38092#issuecomment-4015079666
Raising some concerns from a quick (AI assisted) review:
1. Wrong file — This is landing in kubernetes.mdx, but the content is
generic OAuth
config that applies to any deployment. It deserves its own page or a spot
in the
auth/security docs, not buried mid-Kubernetes-install-guide.
2. Very Keycloak-specific — The URL structure
(/protocol/openid-connect/...), the
`:::note` framing, and even the group key names are all Keycloak-specific.
If it stays,
it should be clearly scoped as a "Keycloak example" rather than a general
OAuth guide
improvement.
3. `:::note` is the wrong container — The admonition wraps setup
instructions, not a
note. It'd be cleaner as a `:::tip` with a header like "Full example:
Keycloak with PKCE
and group mapping" or just as a regular `##` section.
4. PR title — feat: Improve oauth doc should be docs(auth): add Keycloak
PKCE and
group-mapping example or similar.
5. `get_url_for_index` missing `()` — Used in several redirect calls as
`self.appbuilder.get_url_for_index` without calling it. If it's a method
(not a
property), those redirects would go to the function object, not the URL.
Worth
double-checking.
Lemme know what you think :)
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
