-
Notifications
You must be signed in to change notification settings - Fork 2
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
chore: set up FOCA #13
Conversation
Signed-off-by: Prati28 <[email protected]>
Reviewer's Guide by SourceryThis pull request introduces the initial setup for the Tus Storage Handler API. It includes configuration settings in 'config.yaml' and an OpenAPI specification in 'storage_handler.yaml'. The configuration file sets up server parameters, security settings, API specifications, and exception handling. The OpenAPI file defines the API's metadata and a basic GET endpoint that returns a welcome message. File-Level Changes
Tips
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @psankhe28 - I've reviewed your changes - here's some feedback:
Overall Comments:
- The PR title 'feat: added foca' doesn't accurately describe the changes. Consider updating it to reflect the addition of configuration files for Tus Storage Handler.
- Several checklist items are unchecked, including adding tests, updating documentation, and adding type annotations. Please address these points or explain why they're not applicable.
- The PR adds configuration files but lacks implementation code. Is this intentional, or is there more code to be added in this feature?
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟡 Security: 2 issues found
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #13 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 2 3 +1
Lines 8 23 +15
=========================================
+ Hits 8 23 +15
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Prati28 <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good for a start :)
However, I think for "adding FOCA" we want the result to be a service that fires up and is available via Swagger UI etc. Of course no controllers need to be implemented (we just need one stub/dummy) - but we want to be sure that everything is in place to implement the controllers next.
To do that (apart from the individual comments in the code), please also add
- The latest FOCA version as a dependency to
pyproject.toml
- A stub for the
home
controller for theGET /
operation attus_storagehandler/api/elixircloud/cloud_storage_handler/controllers.py
(or replacecloud_storage_handler
withcsh
); note that that FOCA looks for the controller for a given operation by putting together the value ofx-openapi-router-controller
(set in theapi
section inconfig.yaml
; see my comment there) and theoperationId
from the specs). It needs to be either a class or a function. In our case, I think a function will do. Just have it return a 200 response for your integration test (see next point). Don't forget to add__init__.py
to each of the new (sub)packages you create, i.e.,api
,elixircloud
,cloud_storage_handler
(orcsh
). - An API server entry point in
tus_storagehandler/app.py
similar to this one - An integration/end-to-end test for your service. Just write a module
test_operations.py
intests/test_integration
that usespytest
andrequests
to send a request toGET /
and asserts that the response is200
. - A way of firing up the service in the CI workflow before the integration test runs. The most robust is to write a simple
deployment/Dockerfile
similar to this one and adeployment/docker-compose.yaml
file similar to this one (you can omit themongodb
service).
Actually, it would be nice to write a small tutorial on that and add it to the FOCA docs. It might also be useful to perhaps optionally connect this with the Cookiecutter somehow, @JaeAeich - as this is really all just boilerplate/always the same. |
Signed-off-by: Prati28 <[email protected]>
Signed-off-by: Prati28 <[email protected]>
Signed-off-by: Prati28 <[email protected]>
Signed-off-by: Prati28 <[email protected]>
Signed-off-by: Prati28 <[email protected]>
Signed-off-by: Prati28 <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@psankhe28 IK you just asked me help with your failing test, but I had to make below some of below changes to make them run on my local machine. I understand this PR might not have been ready for a review but while checking your code, I just noticed couple of things which I found could be worth looking into so I commented so they don't go unnoticed. But to narrow down why tests are failing:
- Wrong placement of
config.yaml
andtus_storagehandler.yaml
- Your test need the server to be running.
Documentation check is also failing, please run |
Signed-off-by: Prati28 <[email protected]>
Signed-off-by: Prati28 <[email protected]>
Signed-off-by: Prati28 <[email protected]>
Signed-off-by: Prati28 <[email protected]>
Signed-off-by: Prati28 <[email protected]>
Signed-off-by: Prati28 <[email protected]>
Signed-off-by: Prati28 <[email protected]>
Signed-off-by: Prati28 <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks mostly good. Please address the integration test comment, then I think we're good to go
Signed-off-by: Pratiksha Sankhe <[email protected]>
Signed-off-by: Pratiksha Sankhe <[email protected]>
Not a review, but I was testing some templating for CI which required me running precommit in this repo, you're PR should fail pre-commit checks, please configure the repository to require it running all the workflows defined. |
@psankhe28 |
Signed-off-by: Pratiksha Sankhe <[email protected]>
There is no pr-evaluation.yaml. Do you mean pr-validation.yaml? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting really close...
Signed-off-by: Pratiksha Sankhe <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done with changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have marked the locations that need changes to remove support for ENVIRONMENT
. Please double check them and resolve the issues, then also resolve the previous conversation about this topic and finally tag me for a review 🙏
Signed-off-by: Pratiksha Sankhe <[email protected]>
Signed-off-by: Pratiksha Sankhe <[email protected]>
Signed-off-by: Pratiksha Sankhe <[email protected]>
Description
Fixes feat: integration of foca #12
Fixes feat: exceptions in case of errors #14
Checklist
of this project, including, in particular, with regard to any style guidelines
Conventional Commits specification; in particular, it clearly
indicates that a change is a breaking change
using the PR title as the commit message
changed behavior
or updated existing ones (only for Python, TypeScript, etc.)
(Google-style Python docstrings) for all
packages/modules/functions/classes/methods or updated existing ones
works
Comments
Summary by Sourcery
Add configuration and OpenAPI specification files for the TUS storage handler, enabling file handling using TUS and Minio (S3 Storage).
New Features: