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

Bump fastapi[all] from 0.82.0 to 0.89.1 #2246

Merged
merged 22 commits into from
Mar 6, 2023

Conversation

dependabot[bot]
Copy link
Contributor

@dependabot dependabot bot commented on behalf of github Jan 16, 2023

Code Changes from humans

  • replace requests with httpx in the fides client to enable proper mocking
  • fix small things such as correcting response objects that were causing failures, etc.
  • remove a test that checked for tuples as data parameters in requests. This is no longer supported with the switch to httpx

Bumps fastapi[all] from 0.82.0 to 0.89.1.

Release notes

Sourced from fastapi[all]'s releases.

0.89.1

Fixes

  • 🐛 Ignore Response classes on return annotation. PR #5855 by @​Kludex. See the new docs in the PR below.

Docs

Translations

0.89.0

Features

  • ✨ Add support for function return type annotations to declare the response_model. Initial PR #1436 by @​uriyyo.

Now you can declare the return type / response_model in the function return type annotation:

from fastapi import FastAPI
from pydantic import BaseModel
app = FastAPI()
class Item(BaseModel):
name: str
price: float
@​app.get("/items/")
async def read_items() -> list[Item]:
return [
Item(name="Portal Gun", price=42.0),
Item(name="Plumbus", price=32.0),
]

FastAPI will use the return type annotation to perform:

  • Data validation
  • Automatic documentation
    • It could power automatic client generators
  • Data filtering

Before this version it was only supported via the response_model parameter.

... (truncated)

Commits
  • 5905c3f 🔖 Release version 0.89.1
  • 00f3c83 📝 Update release notes
  • e84cb66 📝 Update release notes
  • fb8e908 📝 Update docs and examples for Response Model with Return Type Annotations, a...
  • 6b83525 📝 Update release notes
  • fba7493 🐛 Ignore Response classes on return annotation (#5855)
  • 53973f7 📝 Update release notes
  • 1562592 🌐 Add Turkish translation for docs/tr/docs/tutorial/first_steps.md (#5691)
  • 52a8417 📝 Update release notes
  • 929289b 📝 Add External Link: FastAPI lambda container: serverless simplified (#5784)
  • Additional commits viewable in compare view

Dependabot compatibility score

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting @dependabot rebase.


Dependabot commands and options

You can trigger Dependabot actions by commenting on this PR:

  • @dependabot rebase will rebase this PR
  • @dependabot recreate will recreate this PR, overwriting any edits that have been made to it
  • @dependabot merge will merge this PR after your CI passes on it
  • @dependabot squash and merge will squash and merge this PR after your CI passes on it
  • @dependabot cancel merge will cancel a previously requested merge and block automerging
  • @dependabot reopen will reopen this PR if it is closed
  • @dependabot close will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
  • @dependabot ignore this major version will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
  • @dependabot ignore this minor version will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
  • @dependabot ignore this dependency will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)

@dependabot dependabot bot added dependabot dependencies Pull requests that update a dependency file python Pull requests that update Python code labels Jan 16, 2023
@dependabot dependabot bot force-pushed the dependabot/pip/fastapi-all--0.89.1 branch 2 times, most recently from 8cdf888 to 23fdace Compare January 24, 2023 17:33
@ThomasLaPiana ThomasLaPiana self-assigned this Jan 31, 2023
@ThomasLaPiana
Copy link
Contributor

@dependabot rebase

Bumps [fastapi[all]](https://github.com/tiangolo/fastapi) from 0.82.0 to 0.89.1.
- [Release notes](https://github.com/tiangolo/fastapi/releases)
- [Commits](fastapi/fastapi@0.82.0...0.89.1)

---
updated-dependencies:
- dependency-name: fastapi[all]
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
@dependabot dependabot bot force-pushed the dependabot/pip/fastapi-all--0.89.1 branch from 23fdace to 2785c90 Compare January 31, 2023 14:13
@sanders41
Copy link
Contributor

This one fails because the test client changed from using requests to httpx so there are slight differences there. All of the non-test code i think will work without error, and it's just tests that need updating.

@ThomasLaPiana
Copy link
Contributor

small bug fix from: fastapi/fastapi#5859

@ThomasLaPiana
Copy link
Contributor

collect_tests and check_install are working locally 🤔 even in docker

@dependabot @github
Copy link
Contributor Author

dependabot bot commented on behalf of github Feb 13, 2023

A newer version of fastapi[all] exists, but since this PR has been edited by someone other than Dependabot I haven't updated it. You'll get a PR for the updated version as normal once this PR is merged.

@cypress
Copy link

cypress bot commented Feb 13, 2023

Passing run #635 ↗︎

0 3 0 0 Flakiness 0

Details:

Merge 2610956 into 342a5d5...
Project: fides Commit: 1ef5d00354 ℹ️
Status: Passed Duration: 00:50 💡
Started: Mar 6, 2023 7:31 AM Ended: Mar 6, 2023 7:32 AM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@ThomasLaPiana
Copy link
Contributor

@adamsachs would you mind helping me take a look at the ops integration tests?

The crux of the issue here is that the test_client changed to httpx, so the mocking we were doing is broken in places now since httpx is only a vague impersonation of requests.

I'm debating on whether we can jank our way around the differences, or if we should attempt to also replace requests with httpx in the application. I don't know enough the fides_client module to know how hard/easy that would be

@sanders41
Copy link
Contributor

respx is the recommended requests-mockreplacement for https.

Also maybe helpful, a compatibility guide with requests here https://www.python-httpx.org/compatibility/

@ThomasLaPiana
Copy link
Contributor

respx is the recommended requests-mockreplacement for https.

Also maybe helpful, a compatibility guide with requests here https://www.python-httpx.org/compatibility/

Oh, nice call on the compatibility guide!

I don't think respx would help in this case, because it's struggling with the code in the application itself (a parameter that doesn't exist in httpx), and I refactored the one test we had that used requests_mock in the actual test 🤔

@ThomasLaPiana
Copy link
Contributor

@adamsachs tagging you as reviewer here since I believe you were in the deepest on the fides client and that's where I had to make substantive changes, feel free to pull in anyone else you think would be helpful here though!

@codecov
Copy link

codecov bot commented Mar 2, 2023

Codecov Report

Patch coverage: 76.19% and project coverage change: +0.12 🎉

Comparison is base (0e7d3f7) 86.58% compared to head (2610956) 86.71%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2246      +/-   ##
==========================================
+ Coverage   86.58%   86.71%   +0.12%     
==========================================
  Files         290      291       +1     
  Lines       16210    16315     +105     
  Branches     2059     2067       +8     
==========================================
+ Hits        14035    14147     +112     
+ Misses       1792     1781      -11     
- Partials      383      387       +4     
Impacted Files Coverage Δ
...s/api/ops/api/v1/endpoints/connection_endpoints.py 93.45% <ø> (ø)
...c/fides/api/ops/api/v1/endpoints/user_endpoints.py 91.30% <ø> (ø)
...ides/api/ops/service/connectors/fides_connector.py 73.43% <ø> (ø)
.../service/privacy_request/request_runner_service.py 77.15% <ø> (ø)
...s/api/ops/service/connectors/fides/fides_client.py 77.89% <71.42%> (-0.24%) ⬇️
src/fides/cli/utils.py 65.89% <75.00%> (-1.95%) ⬇️
src/fides/lib/oauth/schemas/oauth.py 87.09% <100.00%> (-9.46%) ⬇️
src/fides/cli/commands/view.py 83.87% <0.00%> (-5.02%) ⬇️
src/fides/cli/options.py 92.20% <0.00%> (-2.60%) ⬇️
... and 19 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@adamsachs
Copy link
Contributor

@adamsachs tagging you as reviewer here since I believe you were in the deepest on the fides client and that's where I had to make substantive changes, feel free to pull in anyone else you think would be helpful here though!

sounds good, i'm happy to take a look. i'll probably try to do some manual testing just to be sure, because although we tried to be as comprehensive as we could in automated tests, the fides client (and fides child/parent functionality in general) definitely pushed our limits, and executing some smoke tests will make me feel that much better about it.

@ThomasLaPiana
Copy link
Contributor

@adamsachs tagging you as reviewer here since I believe you were in the deepest on the fides client and that's where I had to make substantive changes, feel free to pull in anyone else you think would be helpful here though!

sounds good, i'm happy to take a look. i'll probably try to do some manual testing just to be sure, because although we tried to be as comprehensive as we could in automated tests, the fides client (and fides child/parent functionality in general) definitely pushed our limits, and executing some smoke tests will make me feel that much better about it.

Excellent, sounds good! Can you comment here with any manual testing you do just so we know for the future where to potentially look for issues?

@adamsachs
Copy link
Contributor

@adamsachs tagging you as reviewer here since I believe you were in the deepest on the fides client and that's where I had to make substantive changes, feel free to pull in anyone else you think would be helpful here though!

sounds good, i'm happy to take a look. i'll probably try to do some manual testing just to be sure, because although we tried to be as comprehensive as we could in automated tests, the fides client (and fides child/parent functionality in general) definitely pushed our limits, and executing some smoke tests will make me feel that much better about it.

Excellent, sounds good! Can you comment here with any manual testing you do just so we know for the future where to potentially look for issues?

yup, will do - as a reference, the "steps to confirm" on the original PR (#1861) for fides connector functionality gives the high-level smoke testing approach/setup. hopefully those commands and setup scripts still work - i'm sure it's been a while since we exercised them :) i'll do that here and make any updates as needed.

Copy link
Contributor

@adamsachs adamsachs left a comment

Choose a reason for hiding this comment

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

@ThomasLaPiana overall this looks good to me - only 2 comments at this point:

  • i pointed out another place i know where we're using requests and requests.Session specifically - do we need to update that as well? your context on the motivation for the change will help here, i'm not 100% sure on why these changes are needed (though i am not doubting that they are needed!)
  • the addition of a timeout in the HTTP client used by FidesClient does introduce an unfortunately sneaky problem that came up in manual testing. i can push up a proposed improvement per my comment - i think it will actually make our code considerably better here. i also will look at whether we can try to get automated test coverage on this case, though it may be a bit tricky (and would be happy to hear suggestions!)

let me know what you think and if you'd like to sync up at all offline to discuss further.

and to document my testing steps (pretty much just pulled from "Steps to Confirm" on #1861):

  • nox -s dev -- child

  • docker exec -ti fides-fides-1 /bin/bash and then python scripts/load_fides_child_examples.py to configure the fides connector config on the "parent" fides

  • configure any valid connector on the "child" fides via API (it should be accessible at http://0.0.0.0:8081 from a host machine) -- here i configured a client-specific ACS connector, which is a good use case because the connector experiences some latency in retrieving results (the real world!). more details for this can be provided offline if needed.

  • submit a privacy request against the "parent" fides and confirm that results are output to whatever storage location is configured on the policy
    e.g.

--header 'Content-Type: application/json' \
--data-raw '[
    {
        "identity": {"email": "[email protected]"},
        "policy_key": "default_access_policy"
    }
]'```

@ThomasLaPiana
Copy link
Contributor

@adamsachs if this looks good to you, can you approve it?

Huge thanks again for the help here :)

@adamsachs adamsachs added the run unsafe ci checks Runs fides-related CI checks that require sensitive credentials label Mar 3, 2023
@adamsachs
Copy link
Contributor

triggered the unsafe CI checks and -- looks like we've got a fundamental bug in our external test setup that prevents it from running at all, and it's gone undetected for over a month 😲 ?! (https://github.com/ethyca/fides/actions/runs/4326442166/jobs/7554058349#step:7:469)

i think the fix should be quick - obviously unrelated to the direct scope of this PR but figure let's just fix it up here and get it merged. i pushed up a fix in 5c29021 that worked for me locally.

@adamsachs
Copy link
Contributor

OK - external saas integration tests were able to run, as were the other external tests. none fully completed successfully, but at least the saas ones did not seem to be impacted by the change in this PR. they completed ~85% of the tests, and most were successful, but the task got cancelled due to what seems like a 15 min timeout. we're used to some intermittent failures on these, so we have to take them with a grain of salt, and it seems very unlikely our changes in this PR have impacted anything negatively there. so i'm good on that front.

but i am a bit concerned about some of the ctl-external tests failures? some seem unrelated, but some seem like they may be due to the change here? (e.g.: https://github.com/ethyca/fides/actions/runs/4326683146/jobs/7554551908#step:6:2054)

the external-datastores test errors seem unrelated, so i'm not concerned about those.

sorry to keep holding this up @ThomasLaPiana, i'd just like your 👀 on the ctl-external tests, since it seems like we may actually need a tweak there. besides that i think we're ready to go.

@ThomasLaPiana
Copy link
Contributor

OK - external saas integration tests were able to run, as were the other external tests. none fully completed successfully, but at least the saas ones did not seem to be impacted by the change in this PR. they completed ~85% of the tests, and most were successful, but the task got cancelled due to what seems like a 15 min timeout. we're used to some intermittent failures on these, so we have to take them with a grain of salt, and it seems very unlikely our changes in this PR have impacted anything negatively there. so i'm good on that front.

but i am a bit concerned about some of the ctl-external tests failures? some seem unrelated, but some seem like they may be due to the change here? (e.g.: https://github.com/ethyca/fides/actions/runs/4326683146/jobs/7554551908#step:6:2054)

the external-datastores test errors seem unrelated, so i'm not concerned about those.

sorry to keep holding this up @ThomasLaPiana, i'd just like your 👀 on the ctl-external tests, since it seems like we may actually need a tweak there. besides that i think we're ready to go.

Thanks for the thorough checks here! The ctl-external tests look like they're missing the CLI login step, I'll add that and at least that failure should be solved

@ThomasLaPiana
Copy link
Contributor

Tests are "fixed" up with one failing AWS test remaining that was known.

Time is still too short for the external SaaS tests but I'm going to open up a follow-up PR to fix both of these

@ThomasLaPiana ThomasLaPiana merged commit 98663d3 into main Mar 6, 2023
@ThomasLaPiana ThomasLaPiana deleted the dependabot/pip/fastapi-all--0.89.1 branch March 6, 2023 08:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependabot dependencies Pull requests that update a dependency file python Pull requests that update Python code run unsafe ci checks Runs fides-related CI checks that require sensitive credentials
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants