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

[ENG-332] [OATHPIT] Throw GitHub into the Oath Pit #389

Merged

Conversation

cslzchen
Copy link
Contributor

@cslzchen cslzchen commented Dec 3, 2019

Ticket

https://openscience.atlassian.net/browse/ENG-332

Purpose

Enable and update the GitHub provider for aiohttp3.

Changes

Code

  • Fix a response release issue for GitHub rate-limiting. In aiohttp3, release() not only releases the response but also closes the connection. This causes exception_from_response() to encounter a connection already closed failure when trying to release or consume the response. The fix: 1) Don't release the response before calling _rl_handle_forbidden_error() and 2) Let _rl_handle_forbidden_error() and exception_from_response() take care of it.

Tests

  • Fixed mock response header value type in tests: int -> str. aiohttpretty requires the str type on both header name and value, which calls .encode() on both when building raw headers. Exceptions would be thrown if int header values were provided.

  • Fixed missing Content-Type header when registering multiple URLs in tests. aiohttp3 is more strict on response content type, where .json() would fail if the content type were not provided or were incorrect. However, the updated aiohttpretty still has the unpleasant limitation that .register_uri() ignores headers (and the body) provided by .register_json_uri() when multiple URL registrations are used. The fix is to explicitly add the required header to the responses param when calling .register_json_uri().

Side effects

No

CR / QA Notes

Here is a list of actions that I tested locally multiple times. Consider a PASS if no extra comment is provided.

  • Getting metadata for a file / folder

  • Downloading

  • DAZ

  • Uploading

    • Upload one file works as expected
    • Upload multiple files at the same time fails. This is an existing bug on staging. Uploading multiple files should be disabled since each upload is independent. The first one gets uploaded successfully but the rest is guaranteed to fail, mostly with waterbutler.core.exceptions.ProviderError: 422, {"documentation_url": "https://developer.github.com/v3", "message": "Update is not a fast forward"} and sometimes with waterbutler.core.exceptions.ProviderError: 422, {"documentation_url": "https://developer.github.com/v3", "message": "Reference cannot be updated"}.
    • Chunked upload: N / A
  • Delete

    • Delete one file fails (an existing bug on staging) with
      waterbutler.core.exceptions.MetadataError: 404, {"documentation_url": "https://developer.github.com/v3/git/commits/#get-a-commit", "message": "Not Found"}
    • Delete one folder works as expected
    • Delete multiple files / folders not allowed by the frontend
  • Folder creation / Upload to folder / Folder deletion

  • Rename files and folders

  • Verifying non-root folder access for id-based folders: N / A

  • Intra move/copy:

    • Nothing happens when moving / copying within the GitHub repo (both OATHPIT and Staging)
  • Inter move/copy (light testing only)

    • Copying (1 file and 1 folder containing 128 files) from GitHub to OSFStorage works as expected.
      • For CR: with the 1 folder of 128 files, I am able to see our rate-limiting handler in effect when there are only about 500 limit left.
      • For QA: please test rate limiting by copying 1 folder containing 32 subfolders, each of which contains 128 files.
    • Copying multiple files or folders is not allowed by the frontend (same with staging)
    • Moving from GitHub to OSFStorage is not allowed by the frontend (same with staging)
    • Copying / moving from OSFStorage to GitHub is not allowed by the frontend (same with staging)
  • Comments persist with moves (light testing only)

  • If enabled, test revisions

  • Project root is storage root vs. a subfolder: N / A

  • Updating a file

Deployment Notes

TBD

@coveralls
Copy link

coveralls commented Dec 3, 2019

Coverage Status

Coverage increased (+7.8%) to 69.341% when pulling 8f6104d on cslzchen:feature/oathpit-github into 3706373 on CenterForOpenScience:feature/oathpit.

aiohttpretty requires the `str` type on both header name and value,
which calls .encode() on both when building raw headers. Exceptions
would be thrown if int header values were provided.
aiohttp3 is more strict on response content-type, where .json()
would fail if the content-type were not provided or were incorrect.
However, the updated aiohttpretty still has the unpleasant bug that
.register_uri() ignores headers provided by .register_json_uri()
when multiple URL registrations are used. The fix is to explicitly
add the required header when calling .register_json_uri().
In aiohttp3, release() not only releases the response but also
closes the connection. This causes exception_from_response()
to encounter a connection already closed failure when trying to
release or consume the response. The fix: 1) Don't release the
response before calling _rl_handle_forbidden_error() and 2) let
_rl_handle_forbidden_error() and exception_from_response() take
care of it.
It doesn't make sense to use register_json_uri() for multi-URL
registration since the extra help this method provides does not
take any effects. Simply use `register_uri()` for this case.
Copy link
Contributor Author

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

PR ready for CR 🔥 🦃 ❄️

tests/providers/github/test_provider.py Show resolved Hide resolved
tests/providers/github/test_provider.py Show resolved Hide resolved
waterbutler/providers/github/provider.py Show resolved Hide resolved
felliott added a commit that referenced this pull request Dec 5, 2019
@felliott felliott merged commit 8f6104d into CenterForOpenScience:feature/oathpit Dec 5, 2019
@felliott
Copy link
Member

felliott commented Dec 5, 2019

LGTM, merged!

@cslzchen cslzchen deleted the feature/oathpit-github branch December 11, 2019 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants