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

Deprecation and Removal of site_configuration #21

Closed
e0d opened this issue Oct 26, 2021 · 12 comments
Closed

Deprecation and Removal of site_configuration #21

e0d opened this issue Oct 26, 2021 · 12 comments
Labels
debt paydown Addresses and fixes prior work-around workflows

Comments

@e0d
Copy link

e0d commented Oct 26, 2021

Narrative

Removing site_configuration, and all references to it across our platform, will increase the speed with which developers can work and reduce cognitive overhead by not having to worry about catering to a little-known, little-used feature. The feature that SC was meant for - microsites/whitelabel - are a concept that edX.org no longer uses or supports.

Goal Remove all references to site_configuration throughout the edX codebase (including edx-platform and other IDAs) to increase developer productivity and decrease cognitive overhead and maintenance.
Reach (Customer) edX and Open edX developers will be impacted by the removal of our legacy site_configuration code, which is littered throughout various codebases. This will speed development and reduce cognitive overhead of writing code.
Impact + Measure (Outcome) Impact: Estimate that a block of code in a larger service/package like this requires ~5-6 weeks/year of maintenance costs.Faster development timelines and improved diagnosis of content issues will be possible by simplifying platform areas that are often poorly or not understood by internal teams.
T&L Gap Impact(FY21 Gap list) This work is not targeting a specific T&L gap, and is more focused on developer pain and development speed.
@e0d e0d added debt paydown Addresses and fixes prior work-around workflows effort: blended labels Oct 29, 2021
@jmakowski1123 jmakowski1123 added the needs definition There are gaps in opportunity scope or definition to be addressed before the opportunity can proceed label Dec 20, 2021
@jmakowski1123 jmakowski1123 removed the needs definition There are gaps in opportunity scope or definition to be addressed before the opportunity can proceed label Dec 28, 2021
@OmarIthawi
Copy link
Member

Thanks @e0d for sharing this.

Is there a way in which we can prevent or delay this deprecation? I'm thinking of few releases after Nutmeg until we can provide a better alternative for a large architectural change.

I understand that there's a cost of ownership and we should probably discuss the consequences of delaying such change.

A number of Open edX providers including Appsembler and eduNEXT uses this feature extensively. This also breaks Figures multi-site mode.

In the long term, we plan also to move away from the site_configuration, but as one can imaging it's much more involved to move out such change when its at the core of our systems.

Here are the alternative I'm thinking of:

  • Turn off the feature by default, put it behind a feature flag so unsuspected new installations won't use it.
  • Assign an owner for the code (Core Contributor) to maintain this feature and review changes in regards to this feature
  • Plan for a removal date in which the feature is removed completely in a date (Open edX release date) that works for the community members who rely on it

Thanks @idegtiarov for scheduling this in the contributor meeting (openedx/wg-coordination#55). I'm interested to hear from both of you and @felipemontoya about the deprecation of this feature and what are your plans.

@sarina
Copy link
Contributor

sarina commented Feb 16, 2022

It would be great if this could be filled out as a Public Engineering DEPR ticket, so that it follows the template and has as much info as possible. @e0d would your be willing to do that? https://github.com/openedx/public-engineering/issues/new/choose

This is per recent updates to OEP-21.

@e0d
Copy link
Author

e0d commented Feb 16, 2022

@sarina I am the creator of this only in the sense that I publicized someone else's proposal. This was originally pitched as BD-47. And, I don't see a ticket on the DEPR board for this, though that was request in the brief as a follow up action.

Arch-BOM was listed as the stakeholder for this proposal on the brief. @jmbowman may have some additional context.

I agree it should be represented on the public DEPR board, but don't think I'm the right person to create the issue.

@OmarIthawi I don't know if there are other stakeholders with reasons to remove this sooner than later, however, I'm glad we are having this discussion. From my point of view the timeline is flexible. Ideally we would have a DEPR ticket and this conversation could happen on it.

As a side note, are AppSembler interested in maintaining site_configuration?

@sarina
Copy link
Contributor

sarina commented Feb 16, 2022

Thanks @e0d - the clarification is appreciated.

I wrote the Blended brief, but I did abandon it well before we got to the part about talking to providers, as I was having trouble finding available tech leadership from the appropriate group. The first task of the pitched Blended project was to create the DEPR ticket itself, and begin gathering community feedback so we could understand how to chart the course of the project. I think beginning this discussion now is more than prudent, as we do have many steps to get through, and getting community input is high up on the list.

@felipemontoya
Copy link
Member

I recently talked to @cmltaWt0 about this in the BTR channel. I have known about this since it was published, but we are actually not very heavy users of this feature. Our multi-tenant approach moved some years ago to a solution built in a custom plugin for this called eox-tenant.

Our solution came to be because we were tired of adding site_configuration support for features in the core one by one. Given that site_config has always been a second class citizen of the edx-platform repo I be happy to help us move into a direction that lets plugins support what site_configuration did and even take it further without requiring the core platform to be mindful of this all the time, specially for developers who don't use the feature. By further I mean things that have never been well covered by site_config such as waffle flags or configuration models.

I'm happy to continue this conversation on the depr ticket .

OmarIthawi added a commit to appsembler/site-configuration-client that referenced this issue Mar 23, 2022
  - prepare for deprecating `site_configuration` openedx/platform-roadmap#21
  - allow easier separation of `secret`, `admin` and other settings
  - packages can use these helpers instead of mocking Open edX
    configuration
OmarIthawi added a commit to appsembler/site-configuration-client that referenced this issue Mar 24, 2022
  - prepare for deprecating `site_configuration` openedx/platform-roadmap#21
  - allow easier separation of `secret`, `admin` and other settings
  - packages can use these helpers instead of mocking Open edX
    configuration
OmarIthawi added a commit to appsembler/site-configuration-client that referenced this issue Mar 28, 2022
  - prepare for deprecating `site_configuration` openedx/platform-roadmap#21
  - allow easier separation of `secret`, `admin` and other settings
  - packages can use these helpers instead of mocking Open edX
    configuration
@feanil
Copy link
Contributor

feanil commented Mar 29, 2022

Looks like this was not added to the DEPR board, I've marked this as proposed so that we can start putting it through the DEPR process. @e0d is there someone already driving this work? That the DEPR group should work with?

@dianakhuang
Copy link

Discussing in the DEPR working group, this functionality is de-facto deprecated that in new systems/MFE are not supporting site-awareness. This is something we will want to be thoughtful about as we get alternatives in place.

@e0d
Copy link
Author

e0d commented Nov 15, 2022

@OmarIthawi What's your current interest in this feature?

@OmarIthawi
Copy link
Member

OmarIthawi commented Nov 16, 2022

Thanks for checking @e0d. We're in the process of moving away from the SiteConfiguraiton models. We're not quiet there yet.

For example, we still depend on the openedx.core.djangoapps.site_configuration.helpers.get_value helper which helps us to override a lot of core configuration values without patching the platform itself.

If it's possible, I suggest the we remove the SiteConfiguration in multiple iterations:

Release 1 -- first batch of breaking changes:

  • Disconnecting the helpers.get_value helper from SiteConfiguration model.
  • Remove the model.
  • Keep a dummy helper, which a fork maintainer can customize until final migration is complete:
    def get_value(name, default=None):
        return getattr(settings, name, default)

Release 2 -- second batch of breaking changes:

  • Wait for fork maintainers to get up to date with the breaking changes, then update the plan
  • Possibly remove the dummy helper altogether and use settings directly

I think this strikes a good balance in removing a rarely used functionality (although we do use it), while also avoid the need for a sudden major refactor.

The main driver for such iterative deprecation is the enormous references for helpers.get_value():

openedx/edx-platform (@ master) $ git grep get_value -- '*.py' | wc -l
447

Please let me know what do you think؟

@feanil feanil removed their assignment Jan 23, 2023
@robrap
Copy link

robrap commented Mar 10, 2023

@robrap
Copy link

robrap commented Sep 14, 2023

Was DEPR ever announced? Also, it is missing a lot of the metadata we collect for actual DEPR tickets. I'm asking because openedx/edx-platform#32656 just came up, and it would be useful to know how to proceed.
FYI: @e0d @dianakhuang

@feanil
Copy link
Contributor

feanil commented Sep 14, 2023

I think the current state is that it's desired but no one can drive this forward. In this case I think it makes sense to allow for improvements since we don't have alternatives that we can provide to the community.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
debt paydown Addresses and fixes prior work-around workflows
Projects
Archived in project
Development

No branches or pull requests

8 participants