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

Update fastapi to 0.65.2 and dependencies #12136

Merged
merged 2 commits into from Jun 15, 2021

Conversation

davelopez
Copy link
Contributor

This supersedes #12128 and upgrades fastapi and also its dependencies as some of them include security fixes:

  • fastapi from 0.63.0 to 0.65.2 see changes here
  • starlette from 0.13.6 to 0.14.2 see changes here
  • pydantic from 1.7.3 to 1.8.2 see changes here

I tried running poetry update but for some reason, in my machine, it keeps running for several hours and seems to be stuck in a loop. In the end, I tried to update the dependencies manually, but maybe someone has more luck/experience with poetry to make it work.

How to test the changes?

  • This is a refactoring of components with existing test coverage.

License

They include several security fixes.
@github-actions github-actions bot added this to the 21.09 milestone Jun 14, 2021
@davelopez davelopez marked this pull request as draft June 14, 2021 13:38
@davelopez
Copy link
Contributor Author

Investigating the failed tests...

@davelopez
Copy link
Contributor Author

Ok, apparently the only breaking change is that now clients that send requests with JSON payload must specify the Content-Type explicitly in the headers, see this comment

It looks like we do so in the POST but not in PUT or DELETE (or any other method that may accept body payload) in the GalaxyInteractorApi. In 3489225 there is a quick&dirty patch to fix the tests, but I guess it would be nice to add the as_json parameters to them as well and maybe even use True as default. Maybe in its own PR?

@davelopez davelopez marked this pull request as ready for review June 15, 2021 08:52
@bgruening
Copy link
Member

Can you add a "todo" to the last commit? I guess this hack needs to go away sooner or later.

Aparentely FastAPI no longer assumes JSON by default.
@davelopez
Copy link
Contributor Author

I guess the timeout in the failed test is unrelated since I only added the TODO comments in the last commit.

@mvdbeek
Copy link
Member

mvdbeek commented Jun 15, 2021

I guess the timeout in the failed test is unrelated since I only added the TODO comments in the last commit.

Yeah, timeout is unrelated. The preceding tests are uploading datasets but are not waiting until the upload is finished, so the job queues up and doesn't finish within 60 seconds. You can see it in the logs, job 21 is the job we're waiting for, while job 17/18/19/20 are still running

@mvdbeek mvdbeek merged commit 95b537f into galaxyproject:dev Jun 15, 2021
@galaxyproject galaxyproject deleted a comment from github-actions bot Jun 15, 2021
mvdbeek added a commit to mvdbeek/galaxy that referenced this pull request Jun 15, 2021
Most methods that stage datasets already do that anwyway. Might be nice
in the future if we could just cancel running jobs at the end of a test
and not wait for uploads where not necessary, but this should increase
test stability in the short term.

Fixes galaxyproject#12136 (comment)
@davelopez davelopez deleted the update_fastapi_0.65.2 branch June 15, 2021 15:01
mvdbeek added a commit to mvdbeek/galaxy that referenced this pull request Jun 15, 2021
Most methods that stage datasets already do that anwyway. Might be nice
in the future if we could just cancel running jobs at the end of a test
and not wait for uploads where not necessary, but this should increase
test stability in the short term.

Fixes galaxyproject#12136 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants