-
Notifications
You must be signed in to change notification settings - Fork 5
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: fix broken CI pipeline #95
Conversation
Reviewer's Guide by SourceryThis pull request deprecates the use of Sequence diagram for CI/CD workflow changessequenceDiagram
participant GH as GitHub Actions
participant Docker as Docker
participant App as Application
Note over GH: Updated deployment flow
GH->>Docker: docker compose up -d --build
Note right of Docker: Replaced 'docker-compose' with 'docker compose'
Docker->>App: Build and start containers
GH->>App: Wait 20s for startup
GH->>App: Probe endpoint
Class diagram showing type hint addition to _validate_run_requestclassDiagram
class RunRequest {
+validate()
}
class dict_atomic {
<<dict>>
+str key
+Any value
}
note for dict_atomic "Added type hint: dict"
RunRequest ..> dict_atomic: uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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 @JaeAeich - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider splitting different types of changes (type hints, dependency updates, docker compose changes) into separate PRs for better review granularity
- What's the rationale for strictly pinning foca to version 0.12.1? Using exact version pinning (==) instead of minimum version (>=) can make security updates more difficult
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 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 and I'll use the feedback to improve your reviews.
@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 @JaeAeich - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider explaining the rationale for pinning pip to version <24.1 in the PR description. If this is addressing a specific issue, it might be better to fix the root cause rather than artificially constraining the pip version.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 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 and I'll use the feedback to improve your reviews.
Generally a good comment, but in this case, we want to repair a broken CI because of outdated code. It makes sense to do so in one PR, especially since the changes are small. |
It may indeed be good to explain this. But no, we're gonna stick with the workaround for now. |
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.
Thanks for splitting up the PRs 👍 Please merge this first, then update the other PR.
And please address the one Sourcery comment by increasing your PR description (but that does not need re-review from my side).
docker-compose
, lint and type hint
pining pip version because of dependency resolution issues in CI, since sooner or later this will be revisited and updated with pyproject, that will eventually update all the deps. |
* minor changes * pin pip version
Summary by Sourcery
Update Docker Compose command, pin pip version to <24.1, and add type hints.
Build:
Pinning pip version because of dependency resolution issues in CI, since sooner or later this will be revisited and updated with pyproject, that will eventually update all the deps.
Chores: