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

Add FastAPI migration of licenses endpoints #11056

Merged
merged 18 commits into from
Jan 8, 2021

Conversation

davelopez
Copy link
Contributor

@davelopez davelopez commented Jan 5, 2021

Related to #10889
I decided to migrate the easiest API endpoints I found as a FastAPI exercise (and to break the ice) :)

Some comments:

  • I decided to add a couple of type annotated methods to the LicensesManager instead of using the existing ones so I can slightly change their behaviour while maintaining the previous controller intact.
  • I assumed that the /api/licenses/{id} endpoint will always be invoked using an SPDX identifier as Path parameter and not an url (the manager supports both). If the URL must be supported the Path parameter should be changed to a Query.

TODO:

  • Add functional tests for the licenses API endpoints

Copy link
Member

@mvdbeek mvdbeek left a comment

Choose a reason for hiding this comment

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

That's looking very nice!

lib/galaxy/managers/licenses.py Outdated Show resolved Hide resolved
lib/galaxy/managers/licenses.py Outdated Show resolved Hide resolved
@jmchilton
Copy link
Member

I decided to add a couple of type annotated methods to the LicensesManager instead of using the existing ones so I can slightly change their behaviour while maintaining the previous controller intact.

This code hasn't been included in a Galaxy release yet - this is probably the best time to get everything perfect and synchronized. For instance, I would move that if license is None: raise ObjectNotFound() right into the manager.

Copy link
Contributor Author

@davelopez davelopez left a comment

Choose a reason for hiding this comment

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

Thanks for the quick review!

@davelopez
Copy link
Contributor Author

I tried to fully document the API endpoints to serve as an example.
Also, I took the liberty to add documentation for the API tags here as explained in the FastAPI documentation.

@davelopez davelopez marked this pull request as ready for review January 7, 2021 13:18
@davelopez davelopez changed the title [WIP] Add FastAPI migration of licenses endpoints Add FastAPI migration of licenses endpoints Jan 7, 2021
@davelopez
Copy link
Contributor Author

Did I break the RolesApiTestCase::test_create_only_admin test somehow? 😳

@mvdbeek
Copy link
Member

mvdbeek commented Jan 7, 2021

I've seen this fail before in other PRs, just one of those flaky tests we'll try fixing on Monday.

@davelopez
Copy link
Contributor Author

Thanks for resolving the conflict! :)

@mvdbeek mvdbeek merged commit 1c89c1e into galaxyproject:dev Jan 8, 2021
@mvdbeek
Copy link
Member

mvdbeek commented Jan 8, 2021

Thank you @davelopez!

@davelopez davelopez deleted the fastapi-migration-licenses branch January 8, 2021 10:03
@jdavcs jdavcs added this to the 21.01 milestone Jan 11, 2021
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.

4 participants