Skip to content

Commit

Permalink
feat!: assume & remove BLOCKSTORE_USE_BLOCKSTORE_APP_API (#33765)
Browse files Browse the repository at this point in the history
Originally, Blockstore was an independent micro-service, accessed via a REST API.
Then, we changed Blockstore so it could be installed as an in-process Django app.

To support both modes, there existed a blockstore_api wrapper library in edx-platform,
with toggles controlling whether the wrapper called out to the micro-service's REST API versus the
Django app's Python API. Now that the micro-service Blockstore implementation is deprecated,
though, this wrapper library and toggles are just unnecessary complexity.

As a first step towards cleanup, we:

* remove several toggles and settings (details below);
* remove the blocokstore_api wrapper methods which called the REST API and
  marshalled them back into Python objects; and
* remove all test cases which relied on the Blockstore micro-service (and were skippped in CI).

In the future, we will remove the content libraries indexer, 
clean up the remaining bits of blockstore_api, and flatten out all
the Blockstore-related test class hierarchies which are no longer nceessary.

BREAKING CHANGE:
* These Django settings are removed:
  * BLOCKSTORE_PUBLIC_URL_ROOT
  * BLOCKSTORE_API_URL
  * BLOCKSTORE_API_AUTH_TOKEN
  * BLOCKSTORE_USE_BLOCKSTORE_APP_API
* The blockstore.use_blockstore_app_api Waffle switch is removed.
* edx-platform will act as it did when the DJango setting BLOCKSTORE_USE_BLOCKSTORE_APP_API
  or the Waffle switch blockstore.use_blockstore_app_api were enabled. That is, any running Blockstore
  micro-service instance will be ignored, and the Blockstore package which is installed into edx-platform
  will be used instead.

Ref: openedx-unsupported/blockstore#296
  • Loading branch information
kdmccormick authored Dec 6, 2023
1 parent 2495120 commit 27803f5
Show file tree
Hide file tree
Showing 19 changed files with 8 additions and 690 deletions.
4 changes: 1 addition & 3 deletions cms/envs/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,6 @@
ENTERPRISE_BACKEND_SERVICE_EDX_OAUTH2_PROVIDER_URL,

# Blockstore
BLOCKSTORE_USE_BLOCKSTORE_APP_API,
BUNDLE_ASSET_STORAGE_SETTINGS,

# Methods to derive settings
Expand Down Expand Up @@ -2606,8 +2605,7 @@
PROCTORING_SETTINGS = {}

################## BLOCKSTORE RELATED SETTINGS #########################
BLOCKSTORE_PUBLIC_URL_ROOT = 'http://localhost:18250'
BLOCKSTORE_API_URL = 'http://localhost:18250/api/v1/'

# Which of django's caches to use for storing anonymous user state for XBlocks
# in the blockstore-based XBlock runtime
XBLOCK_RUNTIME_V2_EPHEMERAL_DATA_CACHE = 'default'
Expand Down
2 changes: 0 additions & 2 deletions cms/envs/devstack-experimental.yml
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,6 @@ AWS_SES_REGION_ENDPOINT: email.us-east-1.amazonaws.com
AWS_SES_REGION_NAME: us-east-1
AWS_STORAGE_BUCKET_NAME: SET-ME-PLEASE (ex. bucket-name)
BASE_COOKIE_DOMAIN: localhost
BLOCKSTORE_API_URL: http://localhost:18250/api/v1
BLOCKSTORE_PUBLIC_URL_ROOT: http://localhost:18250
BLOCK_STRUCTURES_SETTINGS:
COURSE_PUBLISH_TASK_DELAY: 30
TASK_DEFAULT_RETRY_DELAY: 30
Expand Down
6 changes: 0 additions & 6 deletions cms/envs/test.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,6 @@

# import settings from LMS for consistent behavior with CMS
from lms.envs.test import ( # pylint: disable=wrong-import-order
BLOCKSTORE_USE_BLOCKSTORE_APP_API,
BLOCKSTORE_API_URL,
COMPREHENSIVE_THEME_DIRS, # unimport:skip
DEFAULT_FILE_STORAGE,
ECOMMERCE_API_URL,
Expand Down Expand Up @@ -184,10 +182,6 @@
}

############################### BLOCKSTORE #####################################
# Blockstore tests
RUN_BLOCKSTORE_TESTS = os.environ.get('EDXAPP_RUN_BLOCKSTORE_TESTS', 'no').lower() in ('true', 'yes', '1')
BLOCKSTORE_API_URL = os.environ.get('EDXAPP_BLOCKSTORE_API_URL', "http://edx.devstack.blockstore-test:18251/api/v1/")
BLOCKSTORE_API_AUTH_TOKEN = os.environ.get('EDXAPP_BLOCKSTORE_API_AUTH_TOKEN', 'edxapp-test-key')
BUNDLE_ASSET_STORAGE_SETTINGS = dict(
STORAGE_CLASS='django.core.files.storage.FileSystemStorage',
STORAGE_KWARGS=dict(
Expand Down
6 changes: 0 additions & 6 deletions lms/envs/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -5104,12 +5104,6 @@ def _make_locale_paths(settings): # pylint: disable=missing-function-docstring
MAILCHIMP_NEW_USER_LIST_ID = ""

########################## BLOCKSTORE #####################################
BLOCKSTORE_PUBLIC_URL_ROOT = 'http://localhost:18250'
BLOCKSTORE_API_URL = 'http://localhost:18250/api/v1/'

# Disable the Blockstore app API by default.
# See openedx.core.lib.blockstore_api.config for details.
BLOCKSTORE_USE_BLOCKSTORE_APP_API = False

# .. setting_name: XBLOCK_RUNTIME_V2_EPHEMERAL_DATA_CACHE
# .. setting_default: default
Expand Down
2 changes: 0 additions & 2 deletions lms/envs/devstack-experimental.yml
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,6 @@ AWS_SES_REGION_ENDPOINT: email.us-east-1.amazonaws.com
AWS_SES_REGION_NAME: us-east-1
AWS_STORAGE_BUCKET_NAME: SET-ME-PLEASE (ex. bucket-name)
BASE_COOKIE_DOMAIN: localhost
BLOCKSTORE_API_URL: http://localhost:18250/api/v1
BLOCKSTORE_PUBLIC_URL_ROOT: http://localhost:18250
BLOCK_STRUCTURES_SETTINGS:
COURSE_PUBLISH_TASK_DELAY: 30
TASK_DEFAULT_RETRY_DELAY: 30
Expand Down
5 changes: 0 additions & 5 deletions lms/envs/test.py
Original file line number Diff line number Diff line change
Expand Up @@ -557,11 +557,6 @@
derive_settings(__name__)

############################### BLOCKSTORE #####################################
# Blockstore tests
RUN_BLOCKSTORE_TESTS = os.environ.get('EDXAPP_RUN_BLOCKSTORE_TESTS', 'no').lower() in ('true', 'yes', '1')
BLOCKSTORE_USE_BLOCKSTORE_APP_API = not RUN_BLOCKSTORE_TESTS
BLOCKSTORE_API_URL = os.environ.get('EDXAPP_BLOCKSTORE_API_URL', "http://edx.devstack.blockstore-test:18251/api/v1/")
BLOCKSTORE_API_AUTH_TOKEN = os.environ.get('EDXAPP_BLOCKSTORE_API_AUTH_TOKEN', 'edxapp-test-key')
XBLOCK_RUNTIME_V2_EPHEMERAL_DATA_CACHE = 'blockstore' # This must be set to a working cache for the tests to pass
BUNDLE_ASSET_STORAGE_SETTINGS = dict(
STORAGE_CLASS='django.core.files.storage.FileSystemStorage',
Expand Down
11 changes: 0 additions & 11 deletions openedx/core/djangoapps/content_libraries/tests/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,6 @@
from openedx.core.lib import blockstore_api
from openedx.core.lib.blockstore_api.tests.base import (
BlockstoreAppTestMixin,
requires_blockstore,
requires_blockstore_app,
)

# Define the URLs here - don't use reverse() because we want to detect
Expand Down Expand Up @@ -355,15 +353,6 @@ def _get_block_handler_url(self, block_key, handler_name):
return self._api('get', url, None, expect_response=200)["handler_url"]


@requires_blockstore
class ContentLibrariesRestApiBlockstoreServiceTest(_ContentLibrariesRestApiTestMixin, APITestCase):
"""
Base class for Blockstore-based Content Libraries test that use the REST API
and the standalone Blockstore service.
"""


@requires_blockstore_app
class ContentLibrariesRestApiTest(
_ContentLibrariesRestApiTestMixin,
BlockstoreAppTestMixin,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
)
from openedx.core.djangoapps.content_libraries.libraries_index import LibraryBlockIndexer, ContentLibraryIndexer
from openedx.core.djangoapps.content_libraries.tests.base import (
ContentLibrariesRestApiBlockstoreServiceTest,
ContentLibrariesRestApiTest,
elasticsearch_test,
URL_BLOCK_METADATA_URL,
Expand Down Expand Up @@ -1232,16 +1231,6 @@ def test_library_block_delete_event(self):
)


@elasticsearch_test
class ContentLibrariesBlockstoreServiceTest(
ContentLibrariesTestMixin,
ContentLibrariesRestApiBlockstoreServiceTest,
):
"""
General tests for Blockstore-based Content Libraries, using the standalone Blockstore service.
"""


@elasticsearch_test
class ContentLibrariesTest(
ContentLibrariesTestMixin,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@

from openedx.core.djangoapps.content_libraries.libraries_index import ContentLibraryIndexer, LibraryBlockIndexer
from openedx.core.djangoapps.content_libraries.tests.base import (
ContentLibrariesRestApiBlockstoreServiceTest,
ContentLibrariesRestApiTest,
elasticsearch_test,
)
Expand Down Expand Up @@ -181,17 +180,6 @@ def verify_uncommitted_libraries(library_key, has_unpublished_changes, has_unpub
verify_uncommitted_libraries(library_key, False, False)


@override_settings(FEATURES={**settings.FEATURES, 'ENABLE_CONTENT_LIBRARY_INDEX': True})
@elasticsearch_test
class ContentLibraryIndexerBlockstoreServiceTest(
ContentLibraryIndexerTestMixin,
ContentLibrariesRestApiBlockstoreServiceTest,
):
"""
Tests the operation of ContentLibraryIndexer using the standalone Blockstore service.
"""


@override_settings(FEATURES={**settings.FEATURES, 'ENABLE_CONTENT_LIBRARY_INDEX': True})
@elasticsearch_test
class ContentLibraryIndexerTest(
Expand Down Expand Up @@ -303,17 +291,6 @@ def test_crud_block(self):
assert LibraryBlockIndexer.get_items([block['id']]) == []


@override_settings(FEATURES={**settings.FEATURES, 'ENABLE_CONTENT_LIBRARY_INDEX': True})
@elasticsearch_test
class LibraryBlockIndexerBlockstoreServiceTest(
LibraryBlockIndexerTestMixin,
ContentLibrariesRestApiBlockstoreServiceTest,
):
"""
Tests the operation of LibraryBlockIndexer using the standalone Blockstore service.
"""


@override_settings(FEATURES={**settings.FEATURES, 'ENABLE_CONTENT_LIBRARY_INDEX': True})
@elasticsearch_test
class LibraryBlockIndexerTest(
Expand Down
33 changes: 1 addition & 32 deletions openedx/core/djangoapps/content_libraries/tests/test_runtime.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

from completion.test_utils import CompletionWaffleTestMixin
from django.db import connections, transaction
from django.test import LiveServerTestCase, TestCase
from django.test import LiveServerTestCase
from django.utils.text import slugify
from organizations.models import Organization
from rest_framework.test import APIClient
Expand All @@ -16,8 +16,6 @@
from openedx.core.djangoapps.content_libraries import api as library_api
from openedx.core.djangoapps.content_libraries.tests.base import (
BlockstoreAppTestMixin,
requires_blockstore,
requires_blockstore_app,
URL_BLOCK_RENDER_VIEW,
URL_BLOCK_GET_HANDLER_URL,
URL_BLOCK_METADATA_URL,
Expand Down Expand Up @@ -224,14 +222,6 @@ def test_xblock_fields(self):
assert xblock_api.get_block_display_name(block_saved) == 'New Display Name'


@requires_blockstore
class ContentLibraryRuntimeBServiceTest(ContentLibraryRuntimeTestMixin, TestCase):
"""
Tests XBlock runtime using XBlocks in a content library using the standalone Blockstore service.
"""


@requires_blockstore_app
class ContentLibraryRuntimeTest(ContentLibraryRuntimeTestMixin, BlockstoreAppTestMixin, LiveServerTestCase):
"""
Tests XBlock runtime using XBlocks in a content library using the installed Blockstore app.
Expand Down Expand Up @@ -545,14 +535,6 @@ def test_i18n(self):
assert 'Submit' not in dummy_public_view.data['content']


@requires_blockstore
class ContentLibraryXBlockUserStateBServiceTest(ContentLibraryXBlockUserStateTestMixin, TestCase): # type: ignore[misc]
"""
Tests XBlock user state for XBlocks in a content library using the standalone Blockstore service.
"""


@requires_blockstore_app
class ContentLibraryXBlockUserStateTest( # type: ignore[misc]
ContentLibraryXBlockUserStateTestMixin,
BlockstoreAppTestMixin,
Expand Down Expand Up @@ -619,19 +601,6 @@ def get_block_completion_status():
assert get_block_completion_status() == 1


@requires_blockstore
class ContentLibraryXBlockCompletionBServiceTest(
ContentLibraryXBlockCompletionTestMixin,
CompletionWaffleTestMixin,
TestCase,
):
"""
Test that the Blockstore-based XBlocks can track their completion status
using the standalone Blockstore service.
"""


@requires_blockstore_app
class ContentLibraryXBlockCompletionTest(
ContentLibraryXBlockCompletionTestMixin,
CompletionWaffleTestMixin,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
"""

from openedx.core.djangoapps.content_libraries.tests.base import (
ContentLibrariesRestApiBlockstoreServiceTest,
ContentLibrariesRestApiTest,
)

Expand Down Expand Up @@ -109,15 +108,6 @@ def check_download():
check_download()


class ContentLibrariesStaticAssetsBlockstoreServiceTest(
ContentLibrariesStaticAssetsTestMixin,
ContentLibrariesRestApiBlockstoreServiceTest,
):
"""
Tests for static asset files in Blockstore-based Content Libraries, using the standalone Blockstore service.
"""


class ContentLibrariesStaticAssetsTest(
ContentLibrariesStaticAssetsTestMixin,
ContentLibrariesRestApiTest,
Expand Down
12 changes: 0 additions & 12 deletions openedx/core/djangoapps/content_libraries/tests/test_views_lti.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
from openedx.core.djangoapps.content_libraries.constants import PROBLEM

from .base import (
ContentLibrariesRestApiBlockstoreServiceTest,
ContentLibrariesRestApiTest,
URL_LIB_LTI_JWKS,
skip_unless_cms,
Expand Down Expand Up @@ -81,17 +80,6 @@ def test_block_not_found(self):
self._api("get", '/api/libraries/v2/blocks/lb:CL-TEST:libgg:problem:bad-block/lti/', None, expect_response=404)


@override_features(ENABLE_CONTENT_LIBRARIES=True,
ENABLE_CONTENT_LIBRARIES_LTI_TOOL=True)
class LibraryBlockLtiUrlViewBlockstoreServiceTest(
LibraryBlockLtiUrlViewTestMixin,
ContentLibrariesRestApiBlockstoreServiceTest,
):
"""
Test generating LTI URL for a block in a library, using the standalone Blockstore service.
"""


@override_features(ENABLE_CONTENT_LIBRARIES=True,
ENABLE_CONTENT_LIBRARIES_LTI_TOOL=True)
class LibraryBlockLtiUrlViewTest(
Expand Down
21 changes: 1 addition & 20 deletions openedx/core/djangolib/tests/test_blockstore_cache.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,6 @@

from django.test import TestCase
from openedx.core.djangolib.blockstore_cache import BundleCache
from openedx.core.lib.blockstore_api.tests.base import (
BlockstoreAppTestMixin,
requires_blockstore,
requires_blockstore_app,
)
from openedx.core.lib import blockstore_api as api


Expand All @@ -27,7 +22,7 @@ def setUpClass(cls):


@patch('openedx.core.djangolib.blockstore_cache.MAX_BLOCKSTORE_CACHE_DELAY', 0)
class BundleCacheTestMixin(TestWithBundleMixin):
class BundleCacheTestMixin(TestWithBundleMixin, TestCase):
"""
Tests for BundleCache
"""
Expand Down Expand Up @@ -112,17 +107,3 @@ def test_bundle_cache_clear(self):
# Now "clear" the cache, forcing the check of the new version:
cache.clear()
assert cache.get(key1) is None


@requires_blockstore
class BundleCacheBlockstoreServiceTest(BundleCacheTestMixin, TestCase):
"""
Tests BundleCache using the standalone Blockstore service.
"""


@requires_blockstore_app
class BundleCacheTest(BundleCacheTestMixin, BlockstoreAppTestMixin, TestCase):
"""
Tests BundleCache using the installed Blockstore app.
"""
5 changes: 4 additions & 1 deletion openedx/core/lib/blockstore_api/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@
This API does not do any caching; consider using BundleCache or (in
openedx.core.djangolib.blockstore_cache) together with these API methods for
improved performance.
TODO: This wrapper is extraneous now that Blockstore-as-a-service isn't supported.
This whole directory tree should be removed by https://github.com/openedx/blockstore/issues/296.
"""
from blockstore.apps.api.data import (
BundleFileData,
Expand All @@ -16,7 +19,7 @@
BundleFileNotFound,
BundleStorageError,
)
from .methods import (
from blockstore.apps.api.methods import (
# Collections:
get_collection,
create_collection,
Expand Down
13 changes: 0 additions & 13 deletions openedx/core/lib/blockstore_api/config/__init__.py

This file was deleted.

20 changes: 0 additions & 20 deletions openedx/core/lib/blockstore_api/config/waffle.py

This file was deleted.

Loading

0 comments on commit 27803f5

Please sign in to comment.