-
Notifications
You must be signed in to change notification settings - Fork 445
Allow httphandler to have auth and other mediatype #811
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
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #811 +/- ##
==========================================
- Coverage 91.54% 91.35% -0.20%
==========================================
Files 17 17
Lines 1621 1643 +22
==========================================
+ Hits 1484 1501 +17
- Misses 137 142 +5
|
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.
Pull Request Overview
This PR adds support for HTTP authentication and custom Accept headers in the HTTPHandler through environment variables (PAPERMILL_HTTP_AUTH_HEADER and PAPERMILL_HTTP_ACCEPT_HEADER), enabling papermill to read from private repositories like Azure DevOps.
Key Changes:
- Added
_get_auth_kwargs()and_get_read_kwargs()helper methods to construct request headers from environment variables - Modified
read()andwrite()methods to use the new auth and accept header configurations - Added comprehensive test coverage for the new authentication and accept header functionality
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| papermill/iorw.py | Implements helper methods to retrieve auth and accept headers from environment variables and integrates them into HTTP read/write operations |
| papermill/tests/test_iorw.py | Adds three new test cases covering auth-only, accept-header-only, and write-with-auth scenarios |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """Gets the Authorization header from PAPERMILL_HTTP_AUTH_HEADER. | ||
| A valid example value Basic dW5hbWU6cGFzc3dvcmQK""" |
Copilot
AI
Nov 13, 2025
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.
The docstring is missing proper formatting. It should specify that this is a colon (:) after "value" or include it as part of the example. The current text reads "A valid example value Basic..." but should be "A valid example value: Basic..." or structured using proper docstring format with a Parameters or Example section.
| """Gets the Authorization header from PAPERMILL_HTTP_AUTH_HEADER. | |
| A valid example value Basic dW5hbWU6cGFzc3dvcmQK""" | |
| """ | |
| Gets the Authorization header from PAPERMILL_HTTP_AUTH_HEADER. | |
| Example | |
| ------- | |
| A valid example value: Basic dW5hbWU6cGFzc3dvcmQK | |
| """ |
|
|
||
| def test_read_with_auth(self): | ||
| """ | ||
| Tests that the `read` function performs a request to the giving path |
Copilot
AI
Nov 13, 2025
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.
Typo in the docstring: "giving" should be "given". This error appears in the original test at line 312 as well, but since this is new code being added, it should use the correct spelling.
|
|
||
| def test_read_with_accept_header(self): | ||
| """ | ||
| Tests that the `read` function performs a request to the giving path |
Copilot
AI
Nov 13, 2025
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.
Typo in the docstring: "giving" should be "given".
HTTPHandler can now be given auth headers and a different accept media type than application/json
I wanted to use papermill to directly read my git repo (azure devops). Since the repo is not publicly available I needed a way to pass auth to papermill while not breaking any current implementations.
In line with other setups in papermill I added an env variable PAPERMILL_HTTP_AUTH_HEADER that will be passed to requests like this {"headers" {"Authorization": PAPERMILL_HTTP_AUTH_HEADER}}. As such it should work for most auth setups. I only tested it for my use case (base64 encoded basic token).
I then also needed the reply to be "plain/text" instead of "application/json" so also added that option via an env var. PAPERMILL_HTTP_ACCEPT_HEADER
All relevant code is in iorw.py.
Wrote tests for all