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

Migrate groups API to fastAPI #17051

Merged
merged 13 commits into from Nov 23, 2023
Merged

Conversation

arash77
Copy link
Contributor

@arash77 arash77 commented Nov 18, 2023

What did you do?

  • Include an Integration test under test/integration/
  • Refactor the legacy groups API logic so all the logic is contained in the GroupsManager class
  • Create pydantic models for the responses and payloads
  • Create the FastAPI routes that will replace the old ones

related to: #10889

Why did you make this change?

This is part of the efforts to migrate the full Galaxy API to FastAPI see #10889.

How to test the changes?

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

@arash77 arash77 changed the title Migrate groups fastapi Migrate groups API to fastAPI Nov 18, 2023
@arash77 arash77 marked this pull request as ready for review November 18, 2023 11:16
@github-actions github-actions bot added this to the 23.2 milestone Nov 18, 2023
@arash77 arash77 force-pushed the migrate_groups_fastapi branch 2 times, most recently from 73aa6d2 to 3095fda Compare November 18, 2023 11:22
Copy link
Contributor

@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.

Nice first attempt!

Find a couple of comments below explaining a bit some of our conventions.

lib/galaxy/managers/groups.py Outdated Show resolved Hide resolved
lib/galaxy/schema/groups.py Outdated Show resolved Hide resolved
lib/galaxy/webapps/galaxy/api/groups.py Outdated Show resolved Hide resolved
lib/galaxy/webapps/galaxy/api/groups.py Outdated Show resolved Hide resolved
test/integration/test_groups.py Outdated Show resolved Hide resolved
lib/galaxy/schema/groups.py Outdated Show resolved Hide resolved
lib/galaxy/managers/groups.py Outdated Show resolved Hide resolved
lib/galaxy/managers/groups.py Outdated Show resolved Hide resolved
lib/galaxy/schema/groups.py Outdated Show resolved Hide resolved
lib/galaxy/schema/groups.py Outdated Show resolved Hide resolved
lib/galaxy/schema/groups.py Outdated Show resolved Hide resolved
lib/galaxy/schema/groups.py Outdated Show resolved Hide resolved
Copy link
Contributor

@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.

Almost there! Pretty good so far!

lib/galaxy/managers/groups.py Outdated Show resolved Hide resolved
lib/galaxy/managers/groups.py Outdated Show resolved Hide resolved
Copy link
Contributor

@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.

Awesome work!
Congratulations on your first contribution to Galaxy! 🚀

@davelopez
Copy link
Contributor

Looks like there are some conflicts now, I recommend you use rebase to fix the conflict instead of merging the dev branch to have a cleaner list of commits.

@davelopez
Copy link
Contributor

Test failures seem unrelated and fail intermittently in other PRs too.

Thank you @arash77 for addressing all the comments! 👍

@davelopez davelopez merged commit 1ed254f into galaxyproject:dev Nov 23, 2023
48 of 50 checks passed
Copy link

This PR was merged without a "kind/" label, please correct.

@davelopez davelopez added the kind/refactoring cleanup or refactoring of existing code, no functional changes label Nov 23, 2023
@arash77 arash77 deleted the migrate_groups_fastapi branch November 23, 2023 08:50
@mvdbeek mvdbeek modified the milestones: 23.2, 24.0 Dec 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/API area/testing/integration area/testing kind/refactoring cleanup or refactoring of existing code, no functional changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants