-
Notifications
You must be signed in to change notification settings - Fork 7
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: ADR 15 - repo consolidation #292
Conversation
Draft of an ADR to consolidate the Aspects related repositories with a goal of reducing maintenance burden, increasing testability, and simplifying dependencies across the sub-projects.
d8c6637
to
ddedc8b
Compare
|
||
This repo (`openedx-aspects`_) remains the primary Aspects repository where the project decisions and documentation are stored. However, we will also move the Python code for Superset asset management and authentication/authorization into this repository as well as the contents of the `aspects-dbt` and `xapi-db-load` repositories. Those repositories can then be deprecated and archived. | ||
|
||
This repo should will also be used to store any common code or configuration between Tutor and non-Tutor Aspects deployments, such as the Alembic migrations needed to bootstrap the Aspects databases. |
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 repo should will also be used to store any common code or configuration between Tutor and non-Tutor Aspects deployments, such as the Alembic migrations needed to bootstrap the Aspects databases. | |
This repo will also be used to store any common code or configuration between Tutor and non-Tutor Aspects deployments, such as the Alembic migrations needed to bootstrap the Aspects databases. |
|
||
`tutor-contrib-aspects`_ will be updated to use the `openedx-aspects`_ repository as a dependency. This will allow us to keep the Tutor plugin as a separate repository, but will allow us to keep the Python code encapsulated in an easier to test environment, force us to move the Tutor Jinja configuration variables into a traditional configuration layer, where they belong. This will also allow us to greatly simplify the Tutor plugin and separate the concerns of code and configuration. | ||
|
||
Care must be taken to ensure that the extension mechanisms for Superset assets, localization, and authentication / authorization are not broken by this change. |
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.
Some (if not most) of those things will be broken, I think we should keep compatibility with custom DBT repositories, but I'm worried that not all extension mechanisms can be kept.
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'm worried that we may not accomplish full compatibility with current extension mechanisms, but it's fine to break it as this will be major refactor and probably we will find better alternatives to extensions
I think that after some preliminary testing this doesn't end up making sense. The code being developed is run in wildly different contexts (aspects image, superset image, local machine) and putting all of those things and their CI in one place created more confusion for me and more complexity than seems really necessary. There may be a way to consolidate this functionality, but I think this plan confuses concerns too much to pursue right now. So I'm going to close it and if anyone has other feelings about how we might pursue it in a more sane way we should talk about it! |
Draft of an ADR to consolidate the Aspects related repositories with a goal of reducing maintenance burden, increasing testability, and simplifying dependencies across the sub-projects.