-
Notifications
You must be signed in to change notification settings - Fork 28
feat: add contextmanager-based management command observability filter #344
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
base: main
Are you sure you want to change the base?
Changes from 4 commits
f871776
c94b1ca
0daee1a
6864106
50d2094
4b93bcd
75d43a7
6334ec6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| """ | ||
| Package where filters related to management command execution are implemented. | ||
| """ |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,53 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Package where filters related to management command execution are implemented. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from collections.abc import Callable | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from typing import Any | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from openedx_filters.tooling import OpenEdxPublicFilter | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| class ManagementCommandExecutionRequested(OpenEdxPublicFilter): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Filter used to modify the execution of Django management commands. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Purpose: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| This filter is triggered in ``manage.py`` before a management command is | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| executed, allowing pipeline steps to wrap or replace the command runner. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Filter Type: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| org.openedx.platform.management.command.execute.requested.v1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Trigger: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| - Repository: edx/edx-platform | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| - Path: manage.py | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| - Function or Method: __main__ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| filter_type = "org.openedx.platform.management.command.execute.requested.v1" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @classmethod | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def run_filter( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cls, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| command_name: str, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| service_variant: str, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| command_runner: Callable[..., Any], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) -> dict[str, Any]: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This signature is non-conventional. The input args (excluding cls) should map to an output tuple with identical parts:
Suggested change
But also, see my comment on your platform PR: edx/edx-platform#200 (comment) The revised signature would look like this instead:
Suggested change
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I updated this filter to the context-manager pattern so inputs and outputs map positionally as a tuple. The signature is now command_contextmanager, command_name, service_variant -> (command_contextmanager, command_name, service_variant), and the test was updated to assert tuple unpacking accordingly. This also aligns with the platform-side change where manage.py remains the only execution callsite and the filter only wraps execution via a context manager. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Process management command execution arguments through the pipeline. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Arguments: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| command_name (str): name of the management command being executed. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| service_variant (str): service variant, such as lms or cms. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| command_runner (Callable): callable that executes the command. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Returns: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| dict[str, Any]: accumulated pipeline output, including possibly | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| modified command metadata and command runner. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return super().run_pipeline( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| command_name=command_name, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| service_variant=service_variant, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| command_runner=command_runner, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| """ | ||
| Tests for management subdomain filters. | ||
| """ | ||
| from unittest.mock import Mock | ||
|
|
||
| from django.test import TestCase | ||
|
|
||
| from openedx_filters.management.filters import ManagementCommandExecutionRequested | ||
|
|
||
|
|
||
| class TestManagementFilters(TestCase): | ||
| """ | ||
| Test class to verify standard behavior of management filters. | ||
| """ | ||
|
|
||
| def test_management_command_execution_requested(self): | ||
| """ | ||
| Test ManagementCommandExecutionRequested filter behavior. | ||
|
|
||
| Expected behavior: | ||
| - The filter should return management command metadata and runner. | ||
| """ | ||
| command_name = "migrate" | ||
| service_variant = "lms" | ||
| command_runner = Mock() | ||
|
|
||
| result = ManagementCommandExecutionRequested.run_filter( | ||
| command_name=command_name, | ||
| service_variant=service_variant, | ||
| command_runner=command_runner, | ||
| ) | ||
|
|
||
| self.assertEqual(command_name, result.get("command_name")) | ||
| self.assertEqual(service_variant, result.get("service_variant")) | ||
| self.assertEqual(command_runner, result.get("command_runner")) |
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.
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.
+1
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.
Updated