Skip to content
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: allow specifying API keys globally for site and instance #8

Merged
merged 4 commits into from
Mar 5, 2025

Conversation

Agrendalath
Copy link
Member

@Agrendalath Agrendalath commented Feb 26, 2025

Description

This implements the option to specify the API key for each model via:

  1. Django admin (Site Configurations).
  2. Django settings.

Testing instructions

  1. If you are using Tutor, you can create the following YAML plugin to test this:
    name: dev_config
    version: 0.1.0
    patches:
      openedx-common-settings: |
        SITE_CACHE_TTL = 0  # To avoid cache delays after updating Site Configuration.
        XBLOCK_SETTINGS = {
           "ai_eval": {
               "GPT4O_API_KEY": "your-openai-api-key",
            }
        }
  2. Use the instructions from the README to verify that the functionality works. If anything is missing, we should update them.

Other information

In the first two commits, I:

  1. Dropped the Python 3.8 support, because it didn't support Self. I double-checked that the client does not use this version.
  2. Converted existing tests to pytest to avoid mixing mark.parametrize from test_compat.py and ddt in the same repository. It's a general platform's direction to use pytest tests.

Private-ref: BB-9551

@Agrendalath Agrendalath force-pushed the agrendalath/bb-9551-global_ai_keys branch from 4515c70 to 62d02cc Compare February 26, 2025 21:28
@Agrendalath Agrendalath self-assigned this Feb 26, 2025
@Agrendalath Agrendalath force-pushed the agrendalath/bb-9551-global_ai_keys branch 2 times, most recently from 6e7f0db to fa3e7b2 Compare February 26, 2025 22:16
#### Security Considerations

For better security, we recommend using site configuration or Django settings instead of configuring API keys at the
XBlock level. This prevents API keys from being exposed in course exports.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Agrendalath Thank you for noting this!

@ArturGaspar
Copy link
Member

@Agrendalath Looks great! Only one thing is that model_api_url should also be configurable by administrators.

@Agrendalath Agrendalath force-pushed the agrendalath/bb-9551-global_ai_keys branch from fa3e7b2 to daf785b Compare February 28, 2025 23:28
@Agrendalath
Copy link
Member Author

Agrendalath commented Feb 28, 2025

@ArturGaspar, sure thing - added in 7577830.

Edit: a quick heads-up that I had to rebase this PR to convert the unit test from 52fe3e4.

This also fixes model URL field data validation.
@Agrendalath Agrendalath force-pushed the agrendalath/bb-9551-global_ai_keys branch from daf785b to 7577830 Compare February 28, 2025 23:42
@ArturGaspar
Copy link
Member

👍

  • I tested this: Django settings are used when available, XBlock settings are used when configured
  • I read through the code
  • I checked for accessibility issues
  • Includes documentation
  • I made sure any change in configuration variables is reflected in the corresponding client's configuration-secure repository.

@Agrendalath Agrendalath merged commit fda7771 into main Mar 5, 2025
5 checks passed
@Agrendalath Agrendalath deleted the agrendalath/bb-9551-global_ai_keys branch March 5, 2025 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants