8000 Upgrade FastAPI to 0.85.1 by pattisdr · Pull Request #1899 · ethyca/fides · GitHub
[go: up one dir, main page]
Skip to content

Upgrade FastAPI to 0.85.1 #1899

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.

Sign up for GitHub

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

Jump to bottom
Closed
pattisdr wants to merge 3 commits into from
Closed

Upgrade FastAPI to 0.85.1 #1899

pattisdr wants to merge 3 commits into from

Conversation

@pattisdr
Copy link
Contributor
@pattisdr pattisdr commented Nov 29, 2022 •
edited
Loading

Closes #1855

Code Changes

  • Upgrade Fast API to 0.85.1

Steps to Confirm

  • Bring up application server
  • Make several API requests against ctl and ops endpoints

Pre-Merge Checklist

  • All CI Pipelines Succeeded
  • Documentation Updated:
    • documentation complete, or draft/outline provided (tag docs-team to complete/review on this branch)
    • documentation issue created (tag docs-team to complete issue separately)
  • Issue Requirements are Met
  • Relevant Follow-Up Issues Created
  • Update CHANGELOG.md

Description Of Changes

Upgrade Fast API. Fast API 0.79 - 0.85 has a bug that doesn't handle strings in OpenAPI status codes fastapi/fastapi#5187.

@pattisdr pattisdr marked this pull request as ready for review November 30, 2022 04:58
sanders41
sanders41 approved these changes Dec 5, 2022
View reviewed changes
Copy link
Contributor
@sanders41 sanders41 left a comment

Choose a reason for hiding this comment

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

0.88.0 is the latest version. Is there are reason not to update to that version?

@pattisdr
Copy link
Contributor Author
pattisdr commented Dec 5, 2022

0.88.0 is the latest version. Is there are reason not to update to that version?

Hi @sanders41 thanks for reviewing! No real reason other than that I've done a lot of testing of this version and no testing of the latest.

@pattisdr
Copy link
Contributor Author
pattisdr commented Dec 5, 2022

Seeing sqlalchemy.exc.TimeoutError: QueuePool limit of size 5 overflow 10 reached, connection timed out, timeout 30.00 (Background on this error at: https://sqlalche.me/e/14/3o7r) after bringing this up-to-date with main, this happened several times - will not be merging until I know what this is.

@sanders41
Copy link
Contributor
sanders41 commented Dec 6, 2022

@pattisdr looks like 0.88.0 is also failing, but for 8000 different reasons #1863. The failures with 0.85.1 I'm not sure what is causing this.

I think I understand the failures in 0.88.0 and know how to fix them. The TestClient was changed from requests to httpx so the methods/arguments need to be updated for httpx.

@pattisdr
Copy link
Contributor Author
pattisdr commented Dec 6, 2022

Thanks for looking into this @sanders41 - yeah it might be best to upgrade all the way. I should have started testing that version first, I had just been looking at this version to see if it would resolve a bug we had on the plus side.

@pattisdr
Copy link
Contributor Author
pattisdr commented Dec 23, 2022

Closing - we should just update to 0.88.0 in #1863.

@pattisdr pattisdr closed this Dec 23, 2022
@pattisdr pattisdr deleted the fides_1855_fastapi_upgrade branch December 23, 2022 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@sanders41 sanders41 sanders41 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Upgrade FastAPI to at least 0.85.1

3 participants

@pattisdr @sanders41
0