refactor(api): move resource initialization into api crate#3864
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Summary by CodeRabbit
WalkthroughThe API crate now initializes shared runtime resources, including Postgres, credentials, certificates, KMS, and optional Vault import state. ChangesRuntime resource initialization
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant API
participant setup_resources
participant Postgres
participant Vault
participant api_core
API->>setup_resources: initialize RuntimeResources
setup_resources->>Postgres: connect and validate pool configuration
setup_resources->>Vault: build clients and optionally import secrets
setup_resources-->>API: return injected resources
API->>api_core: invoke run_core with resources and DynamicSettings
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
ec18c0e to
bfc9e57
Compare
bfc9e57 to
ecdafd9
Compare
ecdafd9 to
9ad4de6
Compare
9ad4de6 to
d665b1d
Compare
|
🌿 Preview your docs: https://nvidia-preview-pull-request-3864.docs.buildwithfern.com/infra-controller |
Move database, Vault, credential, certificate, KMS, and secret-import setup into the api composition crate. Keep api-core focused on API implementation and domain logic. Signed-off-by: Dmitry Porokh <dporokh@nvidia.com>
d665b1d to
ed195f3
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/api-core/src/bootstrap.rs (1)
67-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
%formatting forlisten_address.Avoid eagerly allocating a
Stringand recording it with Debug formatting; pass the existingDisplayvalue directly.Proposed fix
- listen_address = carbide_config.listen.to_string(), + listen_address = %carbide_config.listen,As per coding guidelines, use
%forDisplayvalues in structured tracing fields.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/api-core/src/bootstrap.rs` at line 67, Update the structured tracing field for listen_address in the surrounding bootstrap configuration to use `%` formatting with carbide_config.listen directly, replacing the eager to_string() conversion and Debug-style recording while preserving the existing field name.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/api-core/src/bootstrap.rs`:
- Line 67: Update the structured tracing field for listen_address in the
surrounding bootstrap configuration to use `%` formatting with
carbide_config.listen directly, replacing the eager to_string() conversion and
Debug-style recording while preserving the existing field name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 056220cd-dcf2-4003-a476-35b86c8456f0
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
crates/api-core/Cargo.tomlcrates/api-core/src/bootstrap.rscrates/api-core/src/run.rscrates/api-core/src/setup.rscrates/api/Cargo.tomlcrates/api/src/lib.rscrates/api/src/resources.rscrates/api/src/run.rs
🚧 Files skipped from review as they are similar to previous changes (7)
- crates/api/src/lib.rs
- crates/api-core/Cargo.toml
- crates/api/Cargo.toml
- crates/api/src/run.rs
- crates/api-core/src/setup.rs
- crates/api-core/src/run.rs
- crates/api/src/resources.rs
Reduce the public surface and clarify the boundary of carbide-api-core by keeping service runtime assembly private while exposing a narrow bootstrap interface to the carbide-api composition crate. This is the fourth change in the series: 1. Move logging initialization and the top-level run entry point — #3851. 2. Move metrics initialization, the metrics endpoint, and process task ownership — #3860. 3. Move database, Vault, credentials, certificates, KMS, and secret-import setup — #3864. 4. Consolidate the remaining service runtime assembly behind the hidden bootstrap interface — this PR. This PR replaces the thin run boundary adapter with RuntimeInputs and start_runtime in the existing hidden bootstrap module. Dynamic settings and Redfish client setup remain internal to api-core, alongside API construction, controller startup, listener startup, and readiness signaling. The existing startup behavior and configuration handling are preserved. Dependencies no longer used by api-core are removed, while test-only dependencies move to dev-dependencies. ## Related issues #3851 #3860 #3864 ## Type of Change - [ ] **Add** - New feature or capability - [ ] **Change** - Changes in existing functionality - [ ] **Fix** - Bug fixes - [ ] **Remove** - Removed features or deprecated functionality - [x] **Internal** - Internal changes (refactoring, tests, docs, etc.) ## Breaking Changes - [ ] **This PR contains breaking changes** ## Testing - [ ] Unit tests added/updated - [ ] Integration tests added/updated - [x] Manual testing performed - [ ] No testing required (docs, internal refactor, etc.) ## Additional Notes This PR is stacked on #3864 and should merge after it. Signed-off-by: Dmitry Porokh <dporokh@nvidia.com>
Reduce the scope and growth of
carbide-api-coreby moving process initialization into thecarbide-apicomposition crate. This PR moves database, Vault, credential, certificate, KMS, and secret-import setup without intended functional or configuration changes.This is the third change in a planned series:
runentry point - refactor(api): move logging initialization into api crate #3851.The existing credential routing, certificate configuration, KMS validation, Vault import behavior, and database pool configuration are preserved. Dependencies no longer used by
api-coremove with the implementation.Related issues
#3851
#3860
Type of Change
Breaking Changes
Testing
Additional Notes
This PR is stacked on #3860 and should merge after it.