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

temp: Retirement refactor example #35714

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

bmtcril
Copy link
Contributor

@bmtcril bmtcril commented Oct 23, 2024

This commit shows an example of how we could use Stevedore drivers to handle retirement extension. The entrypoint name would become the "API" in our configuration, and each "API" extension class would provide a "get_instance" method which would take the retirement config dict and return a constructed instance of the API class, which would then be used a normal.

This code is not complete, simply a real world example of how we could abstract out the existing 3rd party APIs using this method.

This commit shows an example of how we could use Stevedore drivers to
handle retirement extension. The entrypoint name would become the "API"
in our configuration, and each "API" extension class would provide a
"get_instance" method which would take the retirement config dict and
return a constructed instance of the API class, which would then be used
a normal.

This code is not complete, simply a real world example of how we could
abstract out the existing 3rd party APIs using this method.
# `retirement_driver` is the namespace chosen by our plugin manager
# this driver should register itself to `retirement_driver`
retirement_driver =
AMPLITUDE = utils.thirdparty_apis.amplitude_api:AmplitudeApi
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The entrypoint names become the "API" from our config, so keeping this AMPLITUDE preserves the existing naming convention in the existing retirement_pipeline config:

- ['RETIRING_AMPLITUDE, 'AMPLITUDE_COMPLETE', 'AMPLITUDE', 'retire_learner']

[options.entry_points]
# `retirement_driver` is the namespace chosen by our plugin manager
# this driver should register itself to `retirement_driver`
retirement_driver =
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any other python packages can be installed that have a setup.cfg like this to register their plugins with the system. They can still be turned on and off by adding or removing the steps in the retirement configuration file.


# Load any Open edX service Apis that are configured
config['LMS'] = LmsApi(lms_base_url, lms_base_url, client_id, client_secret)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Open question whether we want to make the 1st party APIs function the same way, but for the sake of simplicity I've left them for now.

)
config[service_name] = mgr.driver.get_instance(config)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These few lines are where the magic happens, if we don't already have an "API" configured (LMS, Ecomm, License Manager, etc would already be configured here) we take the value from our configuration and try to fetch a plugin that has a matching name in the retirement_driver entrypoint namespace. So AMPLITUDE, for instance.

If it exists, the given class will be have get_instance(config) called on it to return a fully configured instance of the class. If it does not exist currently this code will raise an error, if this passes we would probably want to wrap that in something with a much more clear error message.

@@ -36,6 +36,26 @@ def __init__(self, amplitude_api_key, amplitude_secret_key):
self.base_url = "https://amplitude.com/"
self.delete_user_path = "api/2/deletions/users"

@staticmethod
def get_instance(config):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Configuration for each plugin API gets pushed down to the class, which is really where it belongs. For simplicity and backwards compatibility I've left all of the configuration variables the same.

This does involve passing the entire config object with potentially many sets of highly permissioned credentials into each plugin. That allows for things like plugins being able to contact the LMS or ecommerce, but isn't a security best practice. A potential improvement would be to move each plugin's configuration to a separate section and only pass that in. I've opted for backwards compatibility over all other considerations in this first pass.

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.

1 participant