upgrade xblock==2.0 #34398
upgrade xblock==2.0 #34398
Conversation
irtazaakram
force-pushed
the
xblock2
branch
3 times, most recently
from
5d8638f to
b75139f
Compare
irtazaakram
force-pushed
the
xblock2
branch
2 times, most recently
from
22886ad to
1a04fe8
Compare
feanil
left a comment
feanil
left a comment
There was a problem hiding this comment.
Generally makes sense but I'm a little unclear about when the runtime id_generator is updated. I think in general it should be left alone as it is set from the system runtime already. If there is a reason to change it, maybe you could explain why?
| id_generator = ImportIdGenerator(parent_key.context_key) | ||
| def_id = id_generator.create_definition(block_type, slug_hint) | ||
| usage_id = id_generator.create_usage(def_id) | ||
| runtime.id_generator = ImportIdGenerator(parent_key.context_key) |
There was a problem hiding this comment.
Why are we saving this as an attribute of the runtime? I don't think we want to update the runtime id_generator in perpetuity just use a different one here at import time right?
There was a problem hiding this comment.
For this specific case runtime was using SplitMongoIdManager instead of ImportIdGenerator.
Details: Unit test failure : Raw Logs
In general id_generator should be assigned by runtime but for every case where a custom id_generator was required we have to pass it through attribute. i.e. runtime.id_generator
There was a problem hiding this comment.
Ohhhh, that is interesting. I guess it makes sense--the runtime here is the general LMS/CMS runtime (CachingDescriptorSystem), so by default, its id_generator is not going to match ImportSystem's id_generator. Thanks for pointing that out @irtazaakram .
Setting it here is a little awkward, but I don't know of a better solution currently.
In order to contain the impact of this, do you think it would be reasonable to set the original id_generator to a temporary value, and then restore it? If you agree, could you make that change, and add a comment explaining why we need to do it?
original_id_generator = runtime.id_generator
runtime.id_generator = ImportIdGenerator(...)
...
runtime.id_generator = original_id_generator
kdmccormick
left a comment
kdmccormick
left a comment
There was a problem hiding this comment.
Just a couple comments @irtazaakram . Otherwise, I think this is good to go.
| id_generator = ImportIdGenerator(parent_key.context_key) | ||
| def_id = id_generator.create_definition(block_type, slug_hint) | ||
| usage_id = id_generator.create_usage(def_id) | ||
| runtime.id_generator = ImportIdGenerator(parent_key.context_key) |
There was a problem hiding this comment.
Ohhhh, that is interesting. I guess it makes sense--the runtime here is the general LMS/CMS runtime (CachingDescriptorSystem), so by default, its id_generator is not going to match ImportSystem's id_generator. Thanks for pointing that out @irtazaakram .
Setting it here is a little awkward, but I don't know of a better solution currently.
In order to contain the impact of this, do you think it would be reasonable to set the original id_generator to a temporary value, and then restore it? If you agree, could you make that change, and add a comment explaining why we need to do it?
original_id_generator = runtime.id_generator
runtime.id_generator = ImportIdGenerator(...)
...
runtime.id_generator = original_id_generator
kdmccormick
added
the
create-sandbox
kdmccormick
left a comment
kdmccormick
left a comment
There was a problem hiding this comment.
Thanks @irtazaakram . The code looks good, assuming you can fix tests and linting.
Before merging, please either smoke-test this locally, or smoke-test with the sandbox that the open-craft-grove bot will add to your PR soon (username/password is openedx/openedx). Also please a cross-reference link to the DEPR ticket and the original xblock PR when you squash your commits.
| id_generator = ImportIdGenerator(parent_key.context_key) | ||
| def_id = id_generator.create_definition(block_type, slug_hint) | ||
| usage_id = id_generator.create_usage(def_id) | ||
| runtime.id_generator = ImportIdGenerator(parent_key.context_key) |
|
Sandbox deployment successful 🚀 |
|
Sandbox deployment successful 🚀 |
|
Sandbox deployment successful 🚀 |
|
@kdmccormick All the tests have been successfully passed, and I've also tested it in the sandbox environment. I'm going ahead with the merge. |
|
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. |
- [DEPR] xblock.fragment and direct id_generator parameters in xblock.runtime.Runtime public-engineering#15
- [DEPR] xblock.fragment and direct id_generator parameters in xblock.runtime.Runtime XBlock#680