-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Refactor configauth to support multiple extension auth interfaces #12702
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Bogdan Drutu <[email protected]>
27b698f
to
61c9b6d
Compare
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.
This looks fine to me (modulo fixing the tests) and avoids a public API dependency between configauth
and extensionauth
.
Please take a look at #12700. I would like to suggest that extensionauth.HTTPClient use more-descriptive names so that middleware extensions and auth extensions do not conflict. As stated in #12700, the middleware extension uses different names. Still, I would recommend e.g., for HTTP client auth extension, ClientAuthRoundTripper() instead of RoundTripper(). |
I am sorry I missed this :( It is now too late for this since we marked this as 1.x. For next time, could you block the "mark as stable" PR or add a comment on the tracking issue for a particular module if there are changes needed there? |
@mx-psi lots of rushing to this 1.x for these packages... as I said multiple times, a stamp of a tag 1.x does not mean anything if the quality is not there. |
Let me start by saying that, to be clear, I think the suggestion by @jmacd's is a good suggestion. I am frustrated that I did not think about it, and I appreciate him taking the time to state it. However, I strongly disagree that this was 'rushed' in any way, and I don't think delaying things would make things any better in general. Since I assume #12702 (comment) relates to This is the work that was put into place for stabilizing
There was a total of 7 months and 2 days excluding the end date from August 29, 2024 to propose any changes. This was shared publicly, on Slack, meetings, Github, including pinging the approvers group. This was done following a process that was agreed for this. At no point this particular objection was raised, despite the multiple opportunities to do so. Respectfully, I don't think 7 months is 'rushing' it. |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
The only interface that looks a bit strange is the "GetHTTPHandler" which needs the "reqParams" attribute. Would have been better if we actually support "HttpServer" extension auth that returns a Handler maybe...