Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (36)
🚧 Files skipped from review as they are similar to previous changes (18)
📝 WalkthroughWalkthroughAdds type-level dependency support: a new Resource.TypeDependencies() method is added and implemented across resources; ResourceData and state graph logic updated to record and evaluate type-based dependencies; test helpers and tests updated accordingly. Changes
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
f7bc019 to
aaf891b
Compare
9df990e to
ae1a213
Compare
46a8e09 to
924237c
Compare
31a709a to
f647f73
Compare
f0c226e to
13e6ece
Compare
f647f73 to
29af08f
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/internal/resource/state_test.go`:
- Around line 310-311: Update the incorrect inline comment: replace the phrase
"type dependency on resource 1" with "type dependency on testResourceType" so it
correctly explains that resource1 is marked for update because it has a type
dependency on the testResourceType that resource2 belongs to (referencing
resource1, resource2, and testResourceType in the test).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1fb24061-cd74-4244-8cd9-d2f10ecd3881
📒 Files selected for processing (36)
server/internal/database/instance_resource.goserver/internal/database/lag_tracker_commit_ts_resource.goserver/internal/database/node_resource.goserver/internal/database/operations/helpers_test.goserver/internal/database/replication_slot_advance_from_cts_resource.goserver/internal/database/replication_slot_create_resource.goserver/internal/database/replication_slot_resource.goserver/internal/database/subscription_resource.goserver/internal/database/switchover_resource.goserver/internal/database/sync_event_resource.goserver/internal/database/wait_for_sync_event_resource.goserver/internal/filesystem/dir_resource.goserver/internal/monitor/instance_monitor_resource.goserver/internal/monitor/service_instance_monitor_resource.goserver/internal/orchestrator/swarm/check_will_restart.goserver/internal/orchestrator/swarm/etcd_creds.goserver/internal/orchestrator/swarm/mcp_config_resource.goserver/internal/orchestrator/swarm/network.goserver/internal/orchestrator/swarm/patroni_cluster.goserver/internal/orchestrator/swarm/patroni_config.goserver/internal/orchestrator/swarm/patroni_member.goserver/internal/orchestrator/swarm/pgbackrest_config.goserver/internal/orchestrator/swarm/pgbackrest_restore.goserver/internal/orchestrator/swarm/pgbackrest_stanza.goserver/internal/orchestrator/swarm/postgres_certs.goserver/internal/orchestrator/swarm/postgres_service.goserver/internal/orchestrator/swarm/postgres_service_spec.goserver/internal/orchestrator/swarm/scale_service.goserver/internal/orchestrator/swarm/service_instance.goserver/internal/orchestrator/swarm/service_instance_spec.goserver/internal/orchestrator/swarm/service_user_role.goserver/internal/orchestrator/swarm/switchover.goserver/internal/resource/resource.goserver/internal/resource/state.goserver/internal/resource/state_test.goserver/internal/scheduler/scheduled_job_resource.go
13e6ece to
c3bdad2
Compare
29af08f to
bf3dd8b
Compare
c3bdad2 to
40aab30
Compare
bf3dd8b to
0508e04
Compare
40aab30 to
7a71fcb
Compare
0508e04 to
7f89e1e
Compare
7f89e1e to
b6a8fb4
Compare
rshoemaker
left a comment
There was a problem hiding this comment.
Looks good - return nil 👍
one edge-case question below (pun-intended)...
| }) | ||
| addEdge(opts, g, from, to) | ||
| } | ||
| for _, ty := range resource.TypeDependencies { |
There was a problem hiding this comment.
I think this inner-loop may allow a self-referential edge to be added - is that something you think we'll ever need? I'm not sure it would cause an issue for the graph library, but I think it could happen if a resource declared itself as a typedep (again, no idea why anyone would do that...)
There was a problem hiding this comment.
That's a good call-out! There's no good reason to do that, and the graph library panics if you try. I'll add a check and return a nicer error for that. We already return an error in our topological sort for any cycle, so it's just the self-reference case that's missing a friendly error right now.
7a71fcb to
30edbed
Compare
b6a8fb4 to
53593e9
Compare
Adds a new `TypeDependencies` method to the `Resource` interface that enables resources to declare a dependency on all resources of a given type without having to use specific IDs. This is useful for ensuring that a particular resource is only updated after all resources of a particular type have been updated, or that a particular resource should be updated any time a resource of a specific type is updated. This will be used for a pg_service.conf resource in a subsequent PR. PLAT-417
53593e9 to
53394ff
Compare
Summary
Adds a new
TypeDependenciesmethod to theResourceinterface that enables resources to declare a dependency on all resources of a given type without having to use specific IDs. This is useful for ensuring that a particular resource is only updated after all resources of a particular type have been updated, or that a particular resource should be updated any time a resource of a specific type is updated. This will be used for a pg_service.conf resource in a subsequent PR.Testing
There are no functionality changes in this PR.
PLAT-417