-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
feat: Support sending OAuth token to codejail service #34023
base: master
Are you sure you want to change the base?
Conversation
343bc99
to
65ff48c
Compare
This supports having an authenticated codejail service in general, but in particular is to support the temporary use of the LMS as a codejail service for the CMS: #33538 The new settings are all optional, and if not provided, the current behavior does not change.
65ff48c
to
73e7ca8
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.
I think this is worth a simple unit test
Yep, still working on that. |
Oh, I thought this was a different PR. What sort of test are you thinking of -- checking the output of |
Yes |
Ready for re-review. Although, here's a thought -- do we want a waffle flag for rollout? Right now there's a Django setting, but a flag would allow us to do a percentage rollout. (Having both would look weird, of course.) |
Wouldn't it cause problems to have this as a Waffle flag while the remote codejail service is a SettingsToggle? Wouldn't that mean that anything without the waffle flag enabled would just throw errors? |
I was thinking of adding a waffle flag in |
This supports having an authenticated codejail service in general, but in particular is to support the temporary use of the LMS as a codejail service for the CMS: #33538
The new settings are all optional, and if not provided, the current behavior does not change.