albundy83 commented on PR #38092: URL: https://github.com/apache/superset/pull/38092#issuecomment-4015812954
> 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 :) Meanwhile, running CI 🤞 I will update my pull request according to your advice :) About the `get_url_for_index`, it's a property not a method I guess, [here](https://github.com/dpgaspar/Flask-AppBuilder/blob/v5.2.0/flask_appbuilder/security/views.py#L585) an example from the official source code. -- 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]
