-
Notifications
You must be signed in to change notification settings - Fork 76
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
[SVCS-475] Send hook request to OSF for GET requests #307
base: develop
Are you sure you want to change the base?
[SVCS-475] Send hook request to OSF for GET requests #307
Conversation
Removed superfluous return in remote_logging Metadata requests are currently not logged as they need an OSF addition to work correctly.
…ts to track that behavior.
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.
LGTM. I will make a quick secondary PR to fix minor style issues and coding bugs before moving to PCR.
@@ -57,6 +57,10 @@ def serialize(self): | |||
|
|||
return payload | |||
|
|||
def __eq__(self, other: object) -> bool: |
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.
👍 I can't think of a better solution than __eq__
.
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.
I will fix other unrelated test failures due to the __version__
issue as well.
tests/core/test_remote_logging.py
Outdated
from waterbutler.core.log_payload import LogPayload | ||
from waterbutler.core.remote_logging import log_to_callback | ||
|
||
from tests.providers.osfstorage.fixtures import ( |
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.
Fix import style as shown in previous examples.
tests/core/fixtures.py
Outdated
|
||
import pytest | ||
|
||
from tests.utils import MockCoroutine |
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.
from waterbutler.core.path import WaterButlerPath
from waterbutler.core.log_payload import LogPayload
from tests.utils import MockCoroutine
from tests.providers.osfstorage.fixtures import (auth, file_path, file_lineage, provider,
file_metadata_object, file_metadata)
tests/core/test_remote_logging.py
Outdated
mock_time): | ||
|
||
with mock.patch('waterbutler.core.utils.send_signed_request', mock_signed_request): | ||
await log_to_callback('move', log_payload, log_payload) |
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.
In remote_logging.py
, the signature for log_to_callback()
is:
async def log_to_callback(action, source=None, destination=None, start_time=None, errors=[]):
This doesn't look right. I removed the two No, they failed.log_payload
arguments and tests passed as expected.
@@ -0,0 +1,122 @@ | |||
import time |
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.
Fix import order and style according to examples I added above.
tests/core/test_remote_logging.py
Outdated
with pytest.raises(Exception) as exc: | ||
await log_to_callback('upload', log_payload, log_payload) | ||
assert exc.message == 'Callback for upload request failed with {},' \ | ||
' got {"status": "failure"}'.format(MockBadResponse()) |
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.
Curly braces are escaped by using {{
and }}
in python.
tests/server/api/v1/fixtures.py
Outdated
|
||
@pytest.fixture | ||
def source_payload(handler): | ||
return LogPayload(handler.resource, |
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.
Fix style for arguments in function/method call.
|
[SVCS-475] Fix minor bugs and improve style
Ticket
https://openscience.atlassian.net/browse/SVCS-475
Purpose
This PR replaces: #277.
The goal of this PR is to send a hook request for all file requests, including GETs so that the osf-side can better track provider metadata. I've also added some unit tests to improve coverage going forward.
Changes
Modifies the accepted actions for sending hooks and creates groups of parameterized tests so the desired action behavior is more obvious.
Side effects
In order to compare LogPayload instances which aren't returned by the logging function, I've have to modify the eq method of the the LogPayload to compare by serialization instead of the typical instance to instance comparison.
QA Notes
None. This is not a user facing change.
Deployment Notes
Requires osf-side changes to have any utility.