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

Add support for pulling from http location. #61

Merged
merged 13 commits into from
Nov 6, 2024
Merged

Add support for pulling from http location. #61

merged 13 commits into from
Nov 6, 2024

Conversation

plietar
Copy link
Member

@plietar plietar commented Nov 6, 2024

This adds two new location types, "http" and "packit", used to interact with outpack_server and Packit, respectively. The behaviour is based upon the R orderly2 implementation.

Authentication to Packit can be done by providing a Packit JWT, a GitHub PAT or interactively using an OAuth device flow against GitHub. For now, tokens obtained through interactive login are only cached in memory. I need to think about where and how tokens may be saved on disk.

There is no pushing support yet.

@plietar plietar requested a review from richfitz November 6, 2024 13:23
Copy link

codecov bot commented Nov 6, 2024

Codecov Report

Attention: Patch coverage is 99.36709% with 2 lines in your changes missing coverage. Please review.

Project coverage is 99.23%. Comparing base (8275bc5) to head (7706baf).
Report is 14 commits behind head on main.

Files with missing lines Patch % Lines
src/pyorderly/outpack/location.py 80.00% 0 Missing and 1 partial ⚠️
src/pyorderly/outpack/location_packit.py 98.83% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #61      +/-   ##
==========================================
+ Coverage   99.12%   99.23%   +0.10%     
==========================================
  Files          51       55       +4     
  Lines        4012     4313     +301     
  Branches      371      285      -86     
==========================================
+ Hits         3977     4280     +303     
+ Misses         21       18       -3     
- Partials       14       15       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@plietar
Copy link
Member Author

plietar commented Nov 6, 2024

Thoughts on just dropping support for 3.8? It was released 5 years ago, and has just hit EOL last month.

functools.cache which is used to cache the tokens isn't available in that version.

@richfitz
Copy link
Member

richfitz commented Nov 6, 2024

Dropping 3.8 is totally fine

Copy link
Member

@richfitz richfitz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice - this took me a while to remember how to get everything going, which is itself a problem. I've been poking around with uv but it's alarming that to get going with this one needs to install uv or pipx to install hatch to install the dependencies, which seems wild.

This worked first time for hitting packit, the oauth flow is nice though not quite as slick as httr2's and I get what looks like some debug printed:

Logging in to https://packit.dide.ic.ac.uk/training/
Visit https://github.com/login/device and enter the code <3F72-22EC>
DeviceAuthorizationResponse(device_code='fb462686ed23572ca5f1079c0db99a673391f355', user_code='3F72-22EC', verification_uri='https://github.com/login/device', expires_in=899, interval=5)
Logged in successfully

so somewhere within this range: https://github.com/mrc-ide/pyorderly/blob/mrc-5968/src/pyorderly/outpack/location_packit.py#L175-L187 - but it's not obvious what is causing that.

I recognise a lot of the code from the R version, so let's merge this and iterate on the differences.

Up to you how much of the coverage holes you want to fix or ignore

@@ -36,6 +37,18 @@ jobs:
run: |
python -m pip install --upgrade pip
pip install hatch

# Compiling outpack server from source takes a few minutes each time.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

"sphinx-copybutton",
"sphinx-autoapi"
"sphinx-rtd-theme",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We really need to get the docs up to spec at some point :(

@plietar plietar force-pushed the mrc-5968 branch 3 times, most recently from ea346db to a37a2d8 Compare November 6, 2024 16:16
This adds two new location types, "http" and "packit", used to interact
with outpack_server and Packit, respectively. The behaviour is based
upon the R orderly2 implementation.

Authentication to Packit can be done by providing a Packit JWT, a GitHub
PAT or interactively using an OAuth device flow against GitHub. For now,
tokens obtained through interactive login are only cached in memory. I
need to think about where and how tokens may be saved on disk.

There is no pushing support yet.
@plietar plietar merged commit 733a7e0 into main Nov 6, 2024
12 checks passed
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.

2 participants