-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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: upstream block ADR, take 2 #35421
docs: upstream block ADR, take 2 #35421
Conversation
2bd86b3
to
074901c
Compare
@ormsbee @bradenmacdonald This is an updated take on my first upstream_block ADR, plus what I learned from actually implementing it, plus a new approach for limited customizability of downstream blocks. |
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.
Looking good!
""" | ||
... | ||
|
||
def sync_from_upstream(self, *, user: User, apply_updates: bool) -> None: |
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.
same here, I think these should be moved elsewhere or at least marked as @final
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.
Moved
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.
A couple of minor comments. I think it's a really solid ADR overall. I'm not sure that we need some of the low-level code details as part of the ADR itself, but I think it's fine like this as well.
"""), | ||
default=None, scope=Scope.settings, hidden=True, enforce_type=True, | ||
) | ||
downstream_customized = List( |
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.
Maybe Set
instead of List
here?
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.
👍🏻
5cb9ecb
to
1dd016e
Compare
1dd016e
to
3872f14
Compare
@bradenmacdonald @ormsbee This is ready for re-review. |
@bradenmacdonald Did you want to make another pass before I merge? |
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.
@kdmccormick Nice work!
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
2U Release Notice: This PR has been deployed to the edX production environment. |
1 similar comment
2U Release Notice: This PR has been deployed to the edX production environment. |
Fully rendered ADR: https://github.com/kdmccormick/edx-platform/blob/kdmccormick/upstream2/docs/decisions/0020-upstream-downstream.rst