WIP: feat: add view to process webhooks #270
Conversation
* feat: add MediaRendition model * refactor: remove unused field from MediaRendition model
Package supports custom media models via settings so media renditions model should respect this.
* feat: add management command to automate some aws setup * docs: add instructions for manual aws setup * docs: add clarification of permission assignment * docs: use descriptive language rather than prescriptive * docs: use placeholders for account information * docs: clarify iam role setup instructions * fix: handle correct exception on missing boto3 package * refactor: replace settings check with error handling * docs: clarify required environment variables * feat: add boto3 as optional dependency * docs: fix typos * docs: fix typos * docs: add detail * docs: emphasise the importance of default rule name * docs: add clarification about eventbridge target id
* feat: add simple base class for transcoding backends * feat: add utility function to get transcoding backend class * refactor: update transcoding backend class name * fix: formatting * feat: add runtime check for transcoding backend * fix: hoist imports to module level * refactor: remove redundant function to get setting * fix: update logging level and remove debug comments * feat: add method parameters to abstract base class * feat: Add error handling for improperly configured transcoding backend * test: add tests for transcoding backend retrieval utility functions * fix: raise ImproperlyConfigured if backend misconfigured * fix: change exception type raised when transcoding backend import fails
* feat: add MediaTranscodingJob model * fix: swappable dependencies in migrations * refactor: use TextChoices for job status and index job_id
* feat: create transcoding task * fix: make job_id nullable * fix: spacing after docstrings * fix: make boto3 an optional dependency with lazy loading * docs: update required aws permissions * docs: update environment variable requirements * refactor: update environment variable names * fix: add checking for transcoding task in progress * chore: remove old comment * refactor: add job states that mirror aws mediaconvert * refactor: move transcoding base exception to base module * refactor: move common aws functions to utils module * docs: add fixme comments for runtime configuration exceptions * fix: add missing init file * refactor: remove aws client lazy loading and helper * test: add tests for aws utility function * test: add aws backend config tests * test: add s3 upload logic tests * test: refactor test class name to be more descriptive * test: add test for the mediaconvert logic * chore: add logging to transcoding signal handler * docs: reminder to make note of role arn * docs: update iam permissions step and role placeholder * fix: patch botocore in for import caching test * test: add transcoding signal handler tests * test: fix import caching side effect * docs: fix missing placeholder * feat: add exception for iam getrole * test: refactor to test behaviour not implementation * fix: import base exception from backend-agnostic module * chore: remove errant print statement * refactor: add exception for transcoding configuration errors * docs: update exceptions in docstrings * fix: aws configuration command imports * refactor: use media type emun consistently
nimasmi
left a comment
There was a problem hiding this comment.
Review of WIP PR.
Thanks for this @ianmeigh. This looks promising. There are some nice touches too, such as aborting early with guard clauses, and skipping processing already completed jobs (in case we got the update from a different source, I presume?).
| """ | ||
| URL patterns for wagtailmedia webhooks. | ||
|
|
||
| To enable webhooks in your project, include these URLs in your urlconf: |
There was a problem hiding this comment.
Comment: Some projects provide the patterns. E.g. https://django-debug-toolbar.readthedocs.io/en/latest/installation.html#add-the-urls. However, I think that just providing instructions is a good compromise for this initial version.
| """ | ||
| Webhook endpoint for receiving transcoding job status updates. | ||
|
|
||
| This view handles POST requests from the AWS EventBridge API Destination to update |
There was a problem hiding this comment.
Question: If this is AWS-specific (and it really does assume AWS), then shouldn't it be namespaced? Perhaps call it AWSTranscodingWebhookView, and likewise the urlconf path.
| return JsonResponse({"error": "Invalid JSON"}, status=400) | ||
|
|
||
| # extract job data | ||
| detail = payload["detail"] |
There was a problem hiding this comment.
Suggestion: Handle missing detail key? Perhaps not, actually, as there are also other potential exceptions such as outputGroupDetails below. A more comprehensive fix, using e.g. Pydantic, might be better left to a future enhancement.
| job_status = detail.get("status") | ||
| job_metadata = {} | ||
|
|
||
| # When a completed MediaConvert event is sent is includes additional information |
There was a problem hiding this comment.
Suggestion (typo):
| # When a completed MediaConvert event is sent is includes additional information | |
| # When a completed MediaConvert event is sent it includes additional information |
| logger.warning( | ||
| f"Webhook received for unknown job_id: {job_id}", | ||
| extra={"job_id": job_id}, | ||
| ) |
There was a problem hiding this comment.
Suggestion: Use %-substitution syntax for logging entries, as it's evaluated lazily, only if the log entry is used by any logger, rather than every time the line is called.
| logger.warning( | |
| f"Webhook received for unknown job_id: {job_id}", | |
| extra={"job_id": job_id}, | |
| ) | |
| logger.warning( | |
| f"Webhook received for unknown job_id: %s", | |
| job_id, | |
| extra={"job_id": job_id}, | |
| ) |
This change should also be applied to the other log entries in this file using f-strings.
| status = self._map_status(job_status) | ||
| if not status: |
There was a problem hiding this comment.
Suggestion: Assume the best, then handle the error, rather than this explicit extra if statement.
| status = self._map_status(job_status) | |
| if not status: | |
| try: | |
| status = self._map_status(job_status) | |
| except KeyError: |
(this involves modifying self._map_status too)
| { | ||
| "success": True, | ||
| "job_id": job_id, | ||
| "status": status, | ||
| }, |
There was a problem hiding this comment.
Question: Is this response format specified by AWS? What does it do with this information?
Description of the change
This PR adds a view that is exposed to receive and process webhooks, containing MediaConvert Job Events sent by the AWS EventBridge API Destination.
Tickets
VE-17
Dev checklist
Manual testing steps, if applicable