docs: Settings Simplification ADR

[kdmccormick]

https://github.com/kdmccormick/edx-platform/blob/kdmccormick/settings-adr/docs/decisions/0022-settings-simplification.rst

[kdmccormick]

@feanil , could you take a first pass? Once it looks good to you, I'll circulate it more broadly.

[feanil]

A few minor comments but nothing major. I can re-review once those are addressed.

[kdmccormick]

@regisb , your wisdom on this ADR would be invaluable.

[kdmccormick]

Sorry @feanil , forgot to push. Ready now.

[feanil]

Looks good to me now.

[regisb]

Reading this ADR, I found myself nodding and mumbling "yes, yes, ok..." all the way down. I used to tell people that there are just two things which are difficult in Open edX: static assets and settings. I'm grateful that you are tackling the latter after having greatly improved the state of the former.

My comments are mostly questions for my own understanding. As far as I'm concerned this is good to go.

[regisb]

This is a greatly simplified structure, and I'm all in favor of it. But I expect that it's going to create issues for some settings that derive from others.

To clarify, I'm talking about the following scenario:

1. common.py defines SETTING_BASE = ... and DERIVED_SETTING1 = foobar1(SETTING_BASE) and DERIVED_SETTING2 = foobar2(SETTING_BASE).
2. production.py modifies SETTING_BASE. DERIVED_SETTING1 and DERIVED_SETTING2 need to be re-defined.

This is for instance what currently happens with STATIC_ROOT_BASE, from which STATIC_ROOT, WEBPACK_LOADER are derived.

How do you expect we are going to resolve these cases? I see 3 possible approaches:

1. Be dumb about it and duplicate settings in common.py and production.py.
2. Somehow preserve the existing derive_settings mechanism. (by the way, are we going to keep it around?)
3. Refactor settings such that we don't have to duplicate settings.

Approach 3 would probably be the "right" one, but I expect we'll have to make this decision on a case-by-case basis.

(I'm sure you've thought about this but I don't see it reflected in this ADR. Or is it part of the "Remove redundant overrides" paragraph below?)

[kdmccormick]

Great question, silly of me to omit that from the ADR.

Having looked through the modules with @feanil a lot lately, we feel that (1) would add lot more work for operators and (3) is just an infeasibly large amount of work and breaking changes. I don't love having bespoke tooling, but if we have to keep some bespokeness around, I think that derived_settings is pretty tame and well-defined. So I propose (2). Specifically, the three common.py files would have a lot of derived defaults. Lmk what you think.

Relatedly, in preparation for this, I have already made the derived_settings API a bit easier to work with: #36192

[regisb]

I'm OK with some bespoke tooling. Like I said, I expect that we might be able to use approach 3 for some settings. If anything, derive_setting will help us identify settings that should be simplified, and we can take on those further down the road.

[kdmccormick]

I expect that we might be able to use approach 3 for some settings. If anything, derive_setting will help us identify settings that should be simplified, and we can take on those further down the road.

I agree. The dependencies between different settings will become much clearer and easy-to-untangle once we have converted all the production.py-special-cases into explicit common.py Derived settings.

[robrap]

Thank you.

[robrap]

If the production.oy files remain unchanged, can you better explain the breaking change aspect of this, and what we expect to break or not break? Also, maybe note the action to be taken to workaround the breaking change, if applicable?

[kdmccormick]

If someone is directly using common.py, changes to those common.py defaults may be a breaking change to them. I'm not aware of any specific site using common.py directly, but we can't count out the possibility. When we merge the breaking commits, we will include a list of defaults that have changed, which the operators can react to.

I can update the ADR to clarify.

[kdmccormick]

Updated

[kdmccormick]

@feanil @regisb I've made some edits to the structure of the ADR and clarified some details based on Robert's feedback. If you want to re-review before I merge, please do so soon.

[robrap]

Thanks @kylecrawshaw. A couple of thoughts:

1. First, I realized that the term third-party was confusing to me. In the context of the Open edX platform, I'm used to the term third-party being applied to libraries (e.g. Django, DRF) and tools. Even the term third-party provider, I would typically associate with examples like: AWS, Azure, Confluent, etc., and not with Open edX providers, unless specifically designated.

• a. Below you used the phrase "third-party providers (like edx.org)", and that was clear to me, but only because the example was edx.org, and it took me a moment to re-orient myself. I'm wondering if switching this to "Open edX providers (like edx.org)", or something like that, would be more clear?
• b. The reference <third_party_repo> is again ambiguous (on its own, and it is opening this paragraph), and maybe this could be <open_edx_provider_repo>? I also realized that you wanted a term that would cover tools like Tutor. Maybe <open_edx_provider_or_tool_repo>?
• c. Related, there are a bunch of references to third-party settings in this ADR, and now I'm not clear which are third-party library settings, and which are Open edX provider settings? Maybe you could search for third-party through the ADR and double-check whether being slightly more explicit will bring clarity?

2. This step is written as if it is required, but the consequences section better details the possible alternative of using lms/envs/yaml.py. Maybe that was part of my original confusion? Is there some way to make this clear, if I am understanding correctly?

[robrap]

1. How will this sub-decision get made, and will this ADR be updated to reflect that commitment?
2. Could this be made less ambiguous at this time? It seemed to me that the current thought is to support a yaml.py, with the caveat that we might learn something along the way that convinces us to deprecate this and push it to Open edX providers. If this is accurate, could we document it? If not, maybe you add an Out of Scope section to this ADR, and detail that it is unknown whether yaml.py will be supported in the platform, or deprecated in favor of Open edX providers adding in this functionality to their own settings files as needed.

[kdmccormick]

1. How will this sub-decision get made, and will this ADR be updated to reflect that commitment?

The ADR currently says:

Finally, based on what we learn throughout this process, our OEP-45 proposal will either recommend to:

1. ...
2. ...

which I felt was enough commitment and detail for an ADR. As with any OEP update, there will be an opportunity to debate the proposed decisions.

Could this be made less ambiguous at this time? It seemed to me that the current thought is to support a yaml.py, with the caveat that we might learn something along the way that convinces us to deprecate this and push it to Open edX providers. If this is accurate, could we document it? If not, maybe you add an Out of Scope section to this ADR, and detail that it is unknown whether yaml.py will be supported in the platform, or deprecated in favor of Open edX providers adding in this functionality to their own settings files as needed.

I think this is an area where you, I, and @feanil all feel differently. Personally, I have felt, and still feel, that production.py is so complex and bespoke that deprecating it and pushing it to Open edX providers will actually be easier for all involved parties than performing (and reacting to) the surgical breaking changes that'd be necessary to get it to a point of having a simple, well-documented, and generally-useful "yaml.py". But, I could be wrong, and I don't think we will be able to come reach consensus on this until we have already gotten our hands dirty coding. Hence, the ambiguity.

[kdmccormick]

First, I realized that the term third-party was confusing to me. In the context of the Open edX platform, I'm used to the term third-party being applied to libraries (e.g. Django, DRF) and tools. Even the term third-party provider, I would typically associate with examples like: AWS, Azure, Confluent, etc., and not with Open edX providers, unless specifically designated.

You're right, I didn't even realize I was using 3rd-party to mean two different things. I'll update to disambiguate. <open_edx_provider_or_tool_repo> is a good idea.

This step is written as if it is required, but the consequences section better details the possible alternative of using lms/envs/yaml.py. Maybe that was part of my original confusion? Is there some way to make this clear, if I am understanding correctly?

Ah, fair. The uncertainty of whether we implement yaml.py has certainly made this ADR harder to write 😛 I will attempt to clarify that that what we're trying to communicate to providers & tools is: !(we_decide_to_support_yaml && you_decide_to_use_yaml) => you_need_a_custom_settings_file