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

docs: User retirement extensibility ADR #36030

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

Conversation

bmtcril
Copy link
Contributor

@bmtcril bmtcril commented Dec 13, 2024

Description

This ADR proposes a method of adding extensibility to the retirement pipeline, with a minimum amount of disruption to existing pipelines. It has developed from conversations with 2U and others interested in customizing the existing retirement capabilities.

Copy link
Contributor

@ormsbee ormsbee left a comment

Choose a reason for hiding this comment

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

Just a small comment suggesting Django settings over Stevedore.

Decisions
=========

#. We will use Stevedore extensions to enable adding new retirement APIs and steps to an organization's workflow without modifying the core retirement pipeline code. This will allow organizations to extend user retirement by simply installing their custom code as a Stevedore extension and changing `their configuration`_ to take advantage of the new capabilities. There should be no need to fork the edx-platform / user_retirement codebase.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we use Django settings for this? There's no need to be independent of Django, is there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure why we'd pull in a web framework / ORM for a standalone command line script. What would the benefit of using Django settings be here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I had actually assumed that Django was already an assumed dependency. If not, I agree that Stevedore is fine.

This ADR proposes a method of adding extensibility to the retirement
pipeline, with a minimum amount of disruption to existing pipelines.
@bmtcril bmtcril force-pushed the bmtcril/retirement_plugin_adr branch from 9056b5d to 605aea7 Compare January 15, 2025 20:50
Copy link
Contributor

@feanil feanil left a comment

Choose a reason for hiding this comment

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

One note but generally looks good to me.


Scoping driver configuration
----------------------------
Refactoring the retirement configuration to be more secure by scoping the configuration passed in to each driver was considered. This would provide better isolation of secrets between drivers and the default services, but would also make it more difficult to share configuration. For instance some drivers need to access LMS APIs, and doing so would require copying the LMS URL, client id, and client secret for each driver that needed them.
Copy link
Contributor

Choose a reason for hiding this comment

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

From the perspective of an operator, if I need to pass all my secrets to a pipeline driver that someone else in the community wrote, I would be more reticent to use it. One response to this is that any retirement code you're running you should understand. However, this seems like it goes against the principal of least privilege. If you still think this is the best idea, can you say some more about what kind of errors could happen and mention that we're choosing to go intentionally go against the principal of least privilege here.

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.

3 participants