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

FastAPI route for remote files #11211

Merged
merged 4 commits into from
Jan 22, 2021

Conversation

davelopez
Copy link
Contributor

Ref #10889

Summary

  • Added RemoteFilesManager to encapsulate the controller logic.
  • Created the pydantic models
  • Added the new FastAPI endpoints

Additional comments

  • I have added some Enums to control the possible values as the /api/remote_files index method docstrings suggested. This provides validation and shows them in the interactive OpenAPI docs as a nice combo. After that, I realized that the target query parameter can be also a URI so ended up leaving it as an str.
  • The FilesSourcePlugin pydantic model tries to represent the base of any kind of files source plugin. To allow other types of plugins (e. g. dropbox, etc.) to add their own fields to the serialization, and do not break the validation, a little extra configuration was added to this model.

@github-actions github-actions bot added this to the 21.05 milestone Jan 22, 2021
@jmchilton
Copy link
Member

This is lovely - a very solid refactoring that was on my todo list.

@davelopez
Copy link
Contributor Author

Many thanks!

@jmchilton
Copy link
Member

The CircleCI test is broken in dev (working on it) but there is an outside chance that integration selenium test failure might intersect this code so I'm re-running that test.

@jdavcs
Copy link
Member

jdavcs commented Jan 22, 2021

The CircleCI test is broken in dev (working on it) but there is an outside chance that integration selenium test failure might intersect this code so I'm re-running that test.

I had the same failures (mypy errors in packages + integrated selenium test) on my PR after a green run. Unless this is different, it should be unrelated to the PR.

@davelopez
Copy link
Contributor Author

Thank you @jmchilton and @ic4f for checking. I managed to run the failing test locally and seems to pass.
Let's check it again after the weekend when the CI has had some proper rest 😄

@jmchilton
Copy link
Member

Yeah - hopefully #11217 helps track down those integration selenium failures.

@jmchilton jmchilton merged commit 2ebabba into galaxyproject:dev Jan 22, 2021
@davelopez davelopez deleted the fastapi_route_remote_files branch January 23, 2021 14:17
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.

3 participants