Skip to content
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

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

Johnetordoff
Copy link
Contributor

@Johnetordoff Johnetordoff commented Dec 18, 2017

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.

AddisonSchiller and others added 3 commits October 4, 2017 11:40
Removed superfluous return in remote_logging
Metadata requests are currently not logged
as they need an OSF addition to work correctly.
@coveralls
Copy link

coveralls commented Dec 18, 2017

Coverage Status

Coverage increased (+0.4%) to 90.22% when pulling 307b9e8 on Johnetordoff:feature/metadata-logging into 3ec764a on CenterForOpenScience:develop.

@coveralls
Copy link

coveralls commented Dec 18, 2017

Coverage Status

Coverage increased (+0.4%) to 90.22% when pulling f722ad4 on Johnetordoff:feature/metadata-logging into 3ec764a on CenterForOpenScience:develop.

Copy link
Contributor

@cslzchen cslzchen left a 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:
Copy link
Contributor

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__.

Copy link
Contributor

@cslzchen cslzchen left a 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.

from waterbutler.core.log_payload import LogPayload
from waterbutler.core.remote_logging import log_to_callback

from tests.providers.osfstorage.fixtures import (
Copy link
Contributor

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.


import pytest

from tests.utils import MockCoroutine
Copy link
Contributor

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)

mock_time):

with mock.patch('waterbutler.core.utils.send_signed_request', mock_signed_request):
await log_to_callback('move', log_payload, log_payload)
Copy link
Contributor

@cslzchen cslzchen Jan 3, 2018

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 log_payload arguments and tests passed as expected. No, they failed.

@@ -0,0 +1,122 @@
import time
Copy link
Contributor

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.

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())
Copy link
Contributor

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.


@pytest.fixture
def source_payload(handler):
return LogPayload(handler.resource,
Copy link
Contributor

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.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.4%) to 90.316% when pulling 6ff7a99 on Johnetordoff:feature/metadata-logging into cae41fc on CenterForOpenScience:develop.

@cslzchen
Copy link
Contributor

cslzchen commented Jan 3, 2018

Update: secondary PR pending. LGTM, move to PCR. 🎆 🎆

[SVCS-475] Fix minor bugs and improve style
@coveralls
Copy link

coveralls commented Jan 3, 2018

Coverage Status

Coverage increased (+0.4%) to 90.316% when pulling b072577 on Johnetordoff:feature/metadata-logging into cae41fc on CenterForOpenScience:develop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants