-
Notifications
You must be signed in to change notification settings - Fork 8
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
ci: replace Travis CI with GH Actions #60
Conversation
Signed-off-by: Prati28 <[email protected]>
Reviewer's Guide by SourceryThis pull request introduces a new GitHub Actions workflow to automate the testing of all DRS-Filer API endpoints. The workflow is triggered on push and pull request events, and includes steps to build and run Docker Compose, install necessary tools, and execute a series of API endpoint tests to ensure the functionality of the DRS-Filer service. Additionally, the Travis CI configuration has been removed in favor of the new GitHub Actions workflows. 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 and they look great!
Here's what I looked at during the review
- 🟡 General issues: 5 issues found
- 🟢 Security: all looks good
- 🟢 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.
.github/workflows/test.yml
Outdated
|
||
- name: Test POST /service-info | ||
run: | | ||
RESPONSE=$(curl -X POST http://localhost:8080/ga4gh/drs/v1/service-info -H 'Content-Type: application/json' -d '{"contactUrl": "mailto:[email protected]","createdAt": "2024-06-12T12:58:19Z","description": "This service provides...","documentationUrl": "https://docs.myservice.example.com","environment": "test","id": "org.ga4gh.myservice","name": "My project","organization": {"name": "My organization","url": "https://example.com"},"type": {"artifact": "beacon","group": "org.ga4gh","version": "1.0.0"},"updatedAt": "2024-06-12T12:58:19Z","version": "1.0.0"}') |
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.
suggestion: Consider breaking down long JSON payloads
The JSON payload in this POST request is quite long and can be hard to read. Consider breaking it down into multiple lines or using a JSON file to improve readability.
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>
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.
This looks good - thanks a lot. However, we should test whether these workflows actually do what they are supposed to do - without having to merge them first.
For that, you need to ensure that there are no conditions that stop the workflows from running, like:
- restrictions on the conditions under which workflows are triggered
- creating the PR from a feature branch and not a fork (which imposes restrictions on the use of secrets)
Also, please address the Sourcery AI comments - they make a lot of sense 🙏 |
@sourcery-ai review |
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:
- Consider adding some high-level comments in the test.py file to explain the purpose of each test function and any complex logic.
- The Python versions in the linting workflow (3.6, 3.7, 3.8) are quite old. Consider updating to more recent versions for better support and features.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟡 Security: 1 issue found
- 🟡 Testing: 13 issues found
- 🟢 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.
Couple more good Sourcery comments :) |
Description
Added GitHub actions for all drs-filer endpoints.
Fixes # (issue)
#58
Type of change
Please delete options that are not relevant.
Checklist:
Summary by Sourcery
This pull request replaces Travis CI with GitHub Actions for continuous integration, including workflows for building and publishing Docker images, linting, and running tests. Additionally, it introduces new tests for DRS-Filer endpoints.