-
Notifications
You must be signed in to change notification settings - Fork 2
feat(deploy-queue): support listing cells #67
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
Conversation
The action.yml prevented running the `outliers` mode, even though the CLI supported it already. Because it generally works I'd call this a bug in the action metadata.
Our version schemes for dev and prod are disjunct, so it is implicitly passed through that already.
fedordikarev
left a comment
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.
@jcgruenhage I am fine to approve without checking, if all of that is refactor.
Please let me know if there are part you want to have focused review or opinion.
If not, I will approve PR in 30 minutes (7pm CET)
|
@copilot please review the PR |
|
@fedordikarev I've opened a new pull request, #68, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@fedordikarev it's not all refactoring. If you look at the individual commits in here, you should be able to tell which ones are interesting to look at, feel free to skip the five marked as |
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.
Pull Request Overview
This PR adds support for listing cells in the deploy-queue system while performing a significant refactor to reduce technical debt. The refactor reorganizes the codebase into a more modular structure with separate handler modules for different operations.
Key changes:
- Introduces a new
list cellssubcommand to display known cells for a given environment - Refactors code organization by splitting monolithic
lib.rsinto focused modules (handler,model,util,constants) - Adds a materialized view for cells with automatic refresh on deployment insert
Reviewed Changes
Copilot reviewed 17 out of 27 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| deploy-queue/src/lib.rs | Refactored main entry point to use new modular structure with separate handler modules |
| deploy-queue/src/model.rs | Extracted data models (Cell, Deployment, DeploymentState, etc.) into dedicated module |
| deploy-queue/src/handler/mod.rs | Created handler module with core deployment operations |
| deploy-queue/src/handler/fetch.rs | Implements fetch operations for deployments, outliers, and cells |
| deploy-queue/src/handler/list.rs | Implements listing functionality for outliers and cells |
| deploy-queue/src/handler/cancel.rs | Implements cancellation operations for deployments |
| deploy-queue/src/util/mod.rs | Created utility module for database, duration, and GitHub integrations |
| deploy-queue/src/util/duration.rs | Extracted duration conversion utilities with DurationExt trait |
| deploy-queue/src/util/database.rs | Extracted database connection logic |
| deploy-queue/src/util/github.rs | Extracted GitHub Actions output utilities |
| deploy-queue/src/constants.rs | Centralized timeout and retry constants |
| deploy-queue/src/cli.rs | Updated CLI structure to support new list subcommand and refactored cancel options |
| deploy-queue/migrations/0007_cells_materialized_view.sql | Added materialized view for distinct cells with auto-refresh trigger |
| deploy-queue/tests/integration_tests.rs | Updated tests to use new handler API structure |
| deploy-queue/tests/outlier_detection_tests.rs | Updated tests to use new handler API structure |
| deploy-queue/tests/deployment_analytics_tests.rs | Updated tests to use new handler API structure |
| deploy-queue/action.yml | Updated GitHub Action to support new CLI structure |
| deploy-queue/Cargo.toml | Bumped version to 0.6.0 |
Files not reviewed (7)
- deploy-queue/.sqlx/query-1096e32f72fa665a9d7617ebff0493a33882ffc11f77739434bb94b0503f37f6.json: Language not supported
- deploy-queue/.sqlx/query-1283715454efddfcc6620af6d3fd49ee5704a30450a54f1cd313c7357d69f438.json: Language not supported
- deploy-queue/.sqlx/query-305b0466fdb34e01fa1c7029b83fe410002c3b54331c7af27e2e82011b011172.json: Language not supported
- deploy-queue/.sqlx/query-41bca488d310eb8e50fc7150a94342972d3561c060c246bf246812fb4b556b3e.json: Language not supported
- deploy-queue/.sqlx/query-8e59cf17dcf58d773c0de3b1c023944c34484829592744e8bad2fd5fbbd621cd.json: Language not supported
- deploy-queue/.sqlx/query-ee8e3e3db7349201dd037a83afac5c7875d33162e4c6320d18e66a2ff681dc33.json: Language not supported
- deploy-queue/.sqlx/query-fd967284576d89af5aec4ac7c2a14917b4a21d6d205f56a5f98ac93dca1b3b5c.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
2613491 to
80c7611
Compare
80c7611 to
781f934
Compare
fedordikarev
left a comment
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.
lgtm
I went into the codebase to add a subcommand for listing cells. As you can tell by the
git diff --stat, I've gotten somewhat sidetracked with a refactor to get rid of some tech debt.I've split this into smaller commits to make it easier to review.