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

fix: standardize requirements and fixes for marshmallow3+ #98

Merged
merged 6 commits into from
Mar 10, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/CODEOWNERS
Validating CODEOWNERS rules …
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,4 @@
# review when someone opens a pull request.
* @amundsen-io/amundsen-committers

*.py @feng-tao @jinhyukchang @allisonsuarez @dikshathakur3119 @verdan @bolkedebruin @mgorsk1
Copy link
Contributor

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)

Copy link
Author

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.

Copy link
Member

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.

Copy link
Contributor

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.

*.py @feng-tao @jinhyukchang @allisonsuarez @verdan @bolkedebruin @mgorsk1
2 changes: 1 addition & 1 deletion .github/workflows/pull_request.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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 .
Copy link
Author

Choose a reason for hiding this comment

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

using pip anyway as python setup.py install leaves a build/ directory that makes mypy unhappy.

- name: Run python unit tests
run: make test
6 changes: 3 additions & 3 deletions Makefile
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
clean:
clean:
find . -name \*.pyc -delete
find . -name __pycache__ -delete
rm -rf dist/

test_unit:
test_unit:
python -m pytest tests
python3 -bb -m pytest tests

Expand All @@ -16,4 +16,4 @@ mypy:
mypy .


test: test_unit lint mypy
test: test_unit lint mypy
2 changes: 1 addition & 1 deletion amundsen_common/log/http_header_caller_retrieval.py
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Copy link
Author

Choose a reason for hiding this comment

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

4 changes: 2 additions & 2 deletions amundsen_common/models/user.py
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ def _str_no_value(self, s: Optional[str]) -> bool:
return False

@pre_load
def preprocess_data(self, data: Dict[str, Any]) -> Dict[str, Any]:
def preprocess_data(self, data: Dict[str, Any], **kwargs: Any) -> Dict[str, Any]:
if self._str_no_value(data.get('user_id')):
data['user_id'] = data.get('email')

Expand All @@ -81,7 +81,7 @@ def preprocess_data(self, data: Dict[str, Any]) -> Dict[str, Any]:
return data

@validates_schema
def validate_user(self, data: Dict[str, Any]) -> None:
def validate_user(self, data: Dict[str, Any], **kwargs: Any) -> None:
if self._str_no_value(data.get('display_name')):
raise ValidationError('"display_name", "full_name", or "email" must be provided')

Expand Down
6 changes: 1 addition & 5 deletions requirements.txt
Original file line number Diff line number Diff line change
@@ -1,9 +1,5 @@
attrs==19.1.0
flake8==3.7.8
Flask==1.1.1
marshmallow==3.6.0
git+https://www.github.com/hilearn/marshmallow-annotations.git@a7a2dc96932430369bdef36555082df990ed9bef#egg=marshmallow-annotations
mypy==0.761
mypy==0.812
pytest>=4.6
pytest-cov
pytest-mock
Expand Down
11 changes: 8 additions & 3 deletions setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

from setuptools import find_packages, setup

__version__ = '0.8.1'
__version__ = '0.8.2'

setup(
name='amundsen-common',
Expand All @@ -15,6 +15,10 @@
maintainer='Amundsen TSC',
maintainer_email='amundsen-tsc@lists.lfai.foundation',
packages=find_packages(exclude=['tests*']),
# pip ignores dependency_links and only installs the `marshmallow-annotations @ ...` requirement
# specified in install_requires. Unfortunately easy_install which is invoked by `python setup.py install`
# does the opposite. For this reason we include this private fork in both sections so both easy_install and
# pip pick this up.
dependency_links=[
('git+https://www.github.com/hilearn/marshmallow-annotations.git@a7a2dc96932430369bd'
'ef36555082df990ed9bef#egg=marshmallow-annotations')
Expand All @@ -35,9 +39,10 @@
# This will allow for any consuming projects to use this library as
# long as they have a version of pyfoobar equal to or greater than 1.x
# and less than 2.x installed.
'flask>=1.0.2',
'Flask>=1.0.2',
'attrs>=19.0.0',
'marshmallow>=3.0,<=3.6',
'marshmallow-annotations'
'marshmallow-annotations @ git+https://www.github.com/hilearn/marshmallow-annotations.git@a7a2dc96932430369bdef36555082df990ed9bef#egg=marshmallow-annotations' # noqa
],
python_requires=">=3.6",
package_data={'amundsen_common': ['py.typed']},
Expand Down
6 changes: 3 additions & 3 deletions tests/unit/log/test_http_header_caller_retrieval.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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(), \
Copy link
Author

Choose a reason for hiding this comment

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

I am very confused why this was working previously. patch.object returns an AsyncMock by default (see https://docs.python.org/3/library/unittest.mock.html#unittest.mock.patch) and that was not being used correctly.

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')


Expand Down