docs: Update READMEs for Blockstore and Tagstore #204
Conversation
openedx-webhooks
added
core committer
open-source-contribution
|
Thanks for the pull request, @bradenmacdonald! As a core committer in this repo, you can merge this once the pull request is approved per the core committer reviewer requirements and according to the agreement with your edX Champion. |
bradenmacdonald
force-pushed
the
update-readmes
branch
from
cc25bc3
to
b2f1c15
Compare
|
@Agrendalath @pomegranited Would you be able to review this? Jira ticket is MNG-3295. |
There was a problem hiding this comment.
Thank you for updating this README @bradenmacdonald ! I really should have done that as part of moving blockstore into an app!
I've suggested some more changes that clarify this move and the recommended usage in production. Let me know if you want any help rewriting these?
| ------------------------------------------------ | ||
|
|
||
| The easiest way to try out the "Content Libraries v2" feature along with Blockstore is to use the Tutor devstack and | ||
| `this library-authoring MFE Tutor plugin <https://github.com/openedx/frontend-app-library-authoring/pull/50>`_. See that plugin's README for details. |
There was a problem hiding this comment.
Does it make more sense to link to the plugin's README directly once your PR is merged -- it looks like it's close :)
There was a problem hiding this comment.
Yes, I definitely will - but it hasn't merged yet. Just waiting for that.
README.rst
Outdated
| ------------------------------------------------ | ||
|
|
||
| The easiest way to try out the "Content Libraries v2" feature along with Blockstore is to use the Tutor devstack and | ||
| `this library-authoring MFE Tutor plugin <https://github.com/openedx/frontend-app-library-authoring/pull/50>`_. See that plugin's README for details. | ||
|
|
||
| -------------------------- | ||
| Using with Docker Devstack |
There was a problem hiding this comment.
These instructions are for running blockstore as a service, incidentally alongside the docker devstack. Since we can now run blockstore as an app, and the docker devstack is deprecated, do we even need these instructions anymore?
But if we're keeping them, they should be updated. E.g. just below here, it says:
Prerequisite: Have your Open edX
Devstack <https://github.com/openedx/devstack>_ properly installed and working. Your devstack must be tracking themasterbranch ofedx-platform; using Blockstore on an older devstack release is not supported.
But above, we say that it's supported from nutmeg onwards.
The notes about running integration tests are outdated as well, and are ok to remove.
There was a problem hiding this comment.
The preface for "Using in Production" can be changed to:
By default, blockstore is run as an app inside of Open edX. Enable it using the waffle switch
blockstore.use_blockstore_app_api.If you need to run blockstore as a separate service (e.g. for scalability or performance reasons), you can deploy blockstore in production using the
blockstore ansible role <https://github.com/openedx/configuration/tree/master/playbooks/roles/blockstore>_.
...
There was a problem hiding this comment.
Thanks @pomegranited ! I pushed 575c37e with those changes. Let me know your thoughts please :)
|
@bradenmacdonald Thanks for the contribution. |
README.rst
Outdated
| ------------- | ||
| Running Tests | ||
| ------------- | ||
|
|
||
| Unit Tests | ||
| ---------- | ||
|
|
||
| To run the unit tests, get into the blockstore container: | ||
|
|
||
| .. code:: | ||
|
|
||
| make blockstore-shell | ||
|
|
||
|
|
||
| And then run: | ||
|
|
||
| .. code:: | ||
|
|
||
| make test | ||
|
|
||
| To save on overhead while running individual tests, from within the container, you can do: | ||
|
|
||
|
|
||
| .. code:: | ||
|
|
||
| DJANGO_SETTINGS_MODULE=blockstore.settings.test ./manage.py test dotted.path.To.test | ||
|
|
There was a problem hiding this comment.
We can keep this section -- we still need the blockstore docker container and these instructions for running tests.
README.rst
Outdated
| #. Clone this repo and ``cd`` into it. | ||
|
|
||
| #. To start the django development server inside a docker container, run this on | ||
| your host machine: | ||
|
|
||
| .. code:: | ||
|
|
||
| make easyserver | ||
|
|
||
| Blockstore is now running at http://localhost:18250/ . Now we need to configure Studio/LMS to work with it. | ||
|
|
There was a problem hiding this comment.
Can you move these lines down under Running Tests? We should keep that section, and this info on how to run the blockstore docker container, so that people still know how to run tests.
There was a problem hiding this comment.
Is it important to still have that way to run tests? I was under the impression that all the tests are currently run in "app mode" in the edx-platform test suite, and these are only needed to run the tests in "IDA mode".
Actually, can we soon remove "IDA mode" from edx-platform and blockstore entirely, or is there some reason we need to continue to support both options?
There was a problem hiding this comment.
I've but those back in, in a modified form. Please review :)
There was a problem hiding this comment.
Thanks for retaining those test instructions until we can get the standalone service removed.
edx.org is still running blockstore standalone in production, so we shouldn't remove it yet.
There was a problem hiding this comment.
👍 Thank you @bradenmacdonald !
- I tested this by reading through the rendered markup, checking links and code snippets for accuracy.
- I read through the code -- testing and setup details are still accurate.
-
I checked for accessibility issuesN/A - Includes documentation
- Commit structure follows OEP-0051
There was a problem hiding this comment.
@bradenmacdonald, thank you for preparing this, and sorry for the late reply. Looks great!
👍
- I tested this: read the diff, checked the links
- I read through the code
- I checked for accessibility issues: n/a
- Includes documentation
|
@jristau1984: thought you might like to know that bradenmacdonald merged this pull request. |
|
@bradenmacdonald 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
Description
This repository's README was a bit out of date. This PR makes some changes to bring it up to date. More work will be needed in future PRs.
I also reinstated the Tagstore README, per the recent discussion about it on the forum.
Author Comments, Concerns, and Open Questions
Test Instructions
Not sure if any testing is needed. Just look for typos and make sure the links work and the information is accurate.