Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: Settings Simplification ADR #36224
base: master
Are you sure you want to change the base?
docs: Settings Simplification ADR #36224
Changes from all commits
b1d4e46
ba7703e
e32b5c8
108d3ff
f943266
321f3ac
8da96ec
9e5c5da
8a21bf3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 not really clear on the consequences of this for us, and what this will entail. Not sure if it is worth adding some notes under a consequences section.
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 have attempted to explain the consequences in this bullet point. Can you be more specific about what's unclear?
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.
Thanks @kylecrawshaw. A couple of thoughts:
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?<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>
?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?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.
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 whetheryaml.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.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.
The ADR currently says:
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.
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.
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.
Thanks @kylecrawshaw. I too obviously don’t know how this will play out. I’m still left wondering if there is a better way to document all this, including the context in these comments?
One idea is to capture this as its own issue, and have the ADR refer to the issue. This would have two benefits. First, it would push the lack of decision outside of an ADR, which is meant to capture decisions. Second, if you want to capture more context or notes on the subject, it gives a home for that discussion. And it feels more natural to update that issue with more information as it is learned, than this decision record.
That said, if you prefer the current take and don’t like this proposal, I don’t think it is worth discussing further. So, please treat this is a completely non-blocking proposal.