-
Notifications
You must be signed in to change notification settings - Fork 26
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
fix: standardize requirements and fixes for marshmallow3+ #98
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,6 +25,6 @@ jobs: | |
with: | ||
python-version: ${{ matrix.python-version }} | ||
- name: Install dependencies | ||
run: pip3 install -r requirements.txt && pip3 install codecov | ||
run: pip3 install -r requirements.txt && pip3 install codecov && pip install . | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. using pip anyway as |
||
- name: Run python unit tests | ||
run: make test |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,4 +12,4 @@ | |
class HttpHeaderCallerRetrieval(BaseCallerRetriever): | ||
def get_caller(self) -> str: | ||
header_key = flask_app.config.get(CALLER_HEADER_KEY, 'user-agent') | ||
return request.headers.get(header_key, 'UNKNOWN') | ||
return request.headers.get(header_key) or 'UNKNOWN' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Possibly a change due to the minor Flask upgrade: headers is a Headers object (not a dict) and the old mypy annotation was broken. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ | |
|
||
import flask | ||
from mock import patch | ||
from mock import MagicMock | ||
|
||
from amundsen_common.log import http_header_caller_retrieval | ||
from amundsen_common.log.http_header_caller_retrieval import HttpHeaderCallerRetrieval | ||
|
@@ -13,12 +14,11 @@ | |
|
||
|
||
class ActionLogTest(unittest.TestCase): | ||
|
||
def test(self) -> None: | ||
with app.test_request_context(), patch.object(http_header_caller_retrieval, 'request') as mock_request: | ||
with app.test_request_context(), \ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am very confused why this was working previously. |
||
patch.object(http_header_caller_retrieval, 'request', new=MagicMock()) as mock_request: | ||
mock_request.headers.get.return_value = 'foo' | ||
actual = HttpHeaderCallerRetrieval().get_caller() | ||
|
||
self.assertEqual(actual, 'foo') | ||
|
||
|
||
|
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 know Diksha left Lyft, but she's still an Amundsen committer. She's welcome to remove herself if she no longer wants to be associated with the project, and we'll eventually remove any inactive maintainers, but I think this isn't needed quite yet (and should be done in a separate PR anyhow)
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 don't believe she had intended to stay involved in the project, so the cleanup made sense. But I am happy to undo as well.
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 think remove the name doesn't mean remove her to be a committer. And I don't agree with the term
we'll eventually remove any inactive maintainers
(not sure where it comes from). We made it explicit that only the committer could raise the request to not be a committer when we donated the project.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.
@feng-tao ah, you're right, I remembered there being a default demotion to emeritus status due to inactivity in the governance, but that doesn't exist.