fix: standardize requirements and fixes for marshmallow3+ #98
Conversation
Signed-off-by: Dmitriy Kunitskiy <dkunitskiy@lyft.com>
dkunitsk
requested review from
allisonsuarez,
bolkedebruin,
dikshathakur3119,
feng-tao,
jinhyukchang,
mgorsk1,
verdan and
a team
as code owners
Signed-off-by: Dmitriy Kunitskiy <dkunitskiy@lyft.com>
Signed-off-by: Dmitriy Kunitskiy <dkunitskiy@lyft.com>
| @@ -10,4 +10,4 @@ | |||
| # review when someone opens a pull request. | |||
| * @amundsen-io/amundsen-committers | |||
|
|
|||
| *.py @feng-tao @jinhyukchang @allisonsuarez @dikshathakur3119 @verdan @bolkedebruin @mgorsk1 | |||
There was a problem hiding this comment.
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.
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.
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.
@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.
It's not an accepted practice to include unused-by-the-package dependencies in setup.py. Given that what's left in reqs.txt is test-only dependencies, requirements.txt is the right place I believe. |
|
@dkunitsk CI fails with unit test error |
| 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.
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.
| @@ -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.
Possibly a change due to the minor Flask upgrade: headers is a Headers object (not a dict) and the old mypy annotation was broken.
Signed-off-by: Dmitriy Kunitskiy <dkunitskiy@lyft.com>
| @@ -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.
using pip anyway as python setup.py install leaves a build/ directory that makes mypy unhappy.
|
@feng-tao ready for another look and merge (assuming we agreed that cleaning up code owners made sense? i can revert that if not) |
Removal per amundsen-io/amundsencommon#98 (comment) Signed-off-by: Dorian Johnson <2020@dorianj.net>
Removal per amundsen-io/amundsencommon#98 (comment) Signed-off-by: Dorian Johnson <2020@dorianj.net>
Removal per amundsen-io/amundsencommon#98 (comment) Signed-off-by: Dorian Johnson <2020@dorianj.net>
Removal per amundsen-io/amundsencommon#98 (comment) Signed-off-by: Dorian Johnson <2020@dorianj.net>
Removal per amundsen-io/amundsencommon#98 (comment) Signed-off-by: Dorian Johnson <2020@dorianj.net>
Removal per amundsen-io/amundsencommon#98 (comment) Signed-off-by: Dorian Johnson <2020@dorianj.net>
Signed-off-by: Dmitriy Kunitskiy dkunitskiy@lyft.com
Summary of Changes
**kwargsarg to decorated methods per marshmallow3 docs. This was blocking downstream adoption.install_requiresare installed by pip and requirements.txt are for the repo's own functionality. Generally requirements.txt should not repeat the packages in setup.py. Have changed to follow this convention.dependency_linksare NOT followed and the correct way to specify direct url dependencies is by putting them in install_requires using therepo @ git+urlsyntax. Pip19 even removed the--follow-dependency-linksoption.python setup.py installdoes not use pip, it uses setuptools+easy_install which is old school. Turns out that setuptools only knows how to use dependency_links and does not know the syntax above. So I believe the answer is to include marshmallow-annotations in BOTH dependency_links AND install_requires. The former will be forpython3 install setup.pyand the latter for consumers who install this package via pip. The other option is to change this line to runpip install .. But sincepython3 install setup.pyis so ubiquitously encouraged in Amundsen-land I wanted this to continue working.CheckList
Make sure you have checked all steps below to ensure a timely review.