Feature/deferred task correlation #120
Conversation
|
Thanks for the contribution! Unfortunately we can't verify the commit author(s): Drew Shafer <a***@.s.com>. One possible solution is to add that email to your GitHub account. Alternatively you can change your commits to another email and force push the change. After getting your commits associated with your GitHub account, sign the Salesforce.com Contributor License Agreement and this Pull Request will be revalidated. |
dshafer
force-pushed
the
feature/deferred-task-correlation-id
branch
from
2d577ba to
135a747
Compare
tests/machinery/test_base.py
Outdated
| tasks._set_correlation_id = set_correlation_id | ||
| tasks._get_correlation_id = lambda: "cid-sentinel" | ||
| try: | ||
|
|
| "filtered_retry_count_2": 0, | ||
| } | ||
|
|
||
| def test_future_task_runner_sets_cid(self): |
There was a problem hiding this comment.
Do we have a test covering when cid is not present?
There was a problem hiding this comment.
basically every existing test is checking that condition
dshafer
force-pushed
the
feature/deferred-task-correlation-id
branch
from
135a747 to
9bdad90
Compare
| resource_instance = resource_class.objects.get(pk=resource_instance_id) | ||
| endpoint_task = getattr(endpoint_class, endpoint_method_name) | ||
|
|
||
| if correlation_id: |
There was a problem hiding this comment.
Is this guard needed? Shouldn't it be set to None if not set? If it's not Noned, wouldn't a set value leak into the next task because it's stored in thread locals?
update correlation_id if cid module installed Fixes #119
dshafer
force-pushed
the
feature/deferred-task-correlation-id
branch
from
9bdad90 to
21dcb6e
Compare
|
Can you add a CHANGELOG entry @dshafer ? |
| newrelic_agent = None | ||
|
|
||
| try: | ||
| import cid.locals |
There was a problem hiding this comment.
Since we use it in testing, should we add django-cid to our optional-dependencies?
There was a problem hiding this comment.
we're actually mocking it in testing, so it's not necessary. What's the rule for putting things in optional-dependencies? It doesn't seem to simplify installing things with pip
There was a problem hiding this comment.
Oh- maybe you overload the necessary method in testing. I'm trying to think about where, if anywhere, django-cid should go in our listed dependencies in pyproject.toml. Maybe nowhere, and (ideally) we would just document that DDA supports it somewhere (README or docs). That doesn't need to be in this PR.
pyproject.tomlpip installsucceeds with a clean virtualenv