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

Upgrade FastAPI to 0.85.1 #1899

Closed
wants to merge 3 commits into from
Closed

Upgrade FastAPI to 0.85.1 #1899

wants to merge 3 commits into from

Conversation

pattisdr
Copy link
Contributor

@pattisdr pattisdr commented Nov 29, 2022

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 tiangolo/fastapi#5187.

@pattisdr pattisdr marked this pull request as ready for review November 30, 2022 04:58
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

@pattisdr looks like 0.88.0 is also failing, but for 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

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade FastAPI to at least 0.85.1
2 participants