Skip to content

feat(server): rfc0019 green .1/.6/.7 — StoreConfig seam + backend selection - #289

Merged
jensholdgaard merged 3 commits into
mainfrom
rfc0019-green-storeconfig
Jun 23, 2026
Merged

feat(server): rfc0019 green .1/.6/.7 — StoreConfig seam + backend selection#289
jensholdgaard merged 3 commits into
mainfrom
rfc0019-green-storeconfig

Conversation

@jensholdgaard

@jensholdgaard jensholdgaard commented Jun 23, 2026

Copy link
Copy Markdown
Owner

What

RFC 0019 green slice 1: the StoreConfig seam + storage-backend selection from config. Greens the config-layer scenarios .1/.6/.7; the S3 server-level scenarios (.2/.3/.4/.5) stay red until the querier/compactor are migrated onto Store (slice 2/3).

How

  • ourios-parquet: StoreConfig { Local(PathBuf) | S3(S3Config) } + open() -> Result<Store, StoreError> (next to Store/S3Config). Exhaustive enum on purpose — adding a backend should force every consumer to handle it. PartialEq/Eq added to S3Config + StoreConfig.
  • ourios-server: build_store_config(...) resolves OURIOS_STORAGE_BACKEND (local default | s3) + OURIOS_BUCKET_ROOT / OURIOS_S3_{BUCKET,ENDPOINT,REGION,PREFIX}. ServerConfig now carries store: StoreConfig. Credentials are never read in config resolution — they come from the AWS chain inside StoreConfig::open (RFC 0019 §3.4), so a resolution error names only the missing key.
  • main() extracts the local path for the local backend; for s3 it fails fast with a clear "role wiring lands in a later RFC 0019 slice" message (the receiver/compactor/querier still address a local path until slice 2/3, §3.3). The default local path is byte-for-byte unchanged.

Tests

  • .1/.6/.7 are unit tests in src/main.rs (they exercise the private build_store_config/build_config): selection + fail-fast on missing bucket / unknown backend (.1); missing-S3-config error names only the key, no credential leakage (.6); the default local path resolves unchanged (.7). The integration stub file keeps .2.5 ignored.
  • cargo clippy -p ourios-parquet -p ourios-server --all-targets --all-features -- -D warnings clean; the existing receiver/querier/served-binary suites pass (they use the local default); cargo fmt --all --check green.

Note

This is env-var config, consistent with the rest of the server (compaction/receiver/querier are all OURIOS_* env). Whether to add a YAML config file is a separate, cross-cutting decision I'm raising with the maintainer (would be an RFC 0004 amendment); it wouldn't change this StoreConfig resolution, only its input source.

Next

Slice 2: migrate the querier + compactor onto the Store listing wrapper (§3.3), then slice 3: localstack e2e (.2/.3/.4/.5) → flip RFC 0019 green.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Added configurable storage backend selection via OURIOS_STORAGE_BACKEND (defaults to local; supports S3).
    • Introduced expanded storage configuration options for local (OURIOS_BUCKET_ROOT) and S3 (e.g., bucket plus optional endpoint/region/prefix), with stricter validation.
  • Chores

    • Refactored server configuration to use a unified storage model; S3 execution now exits early with a clear “deferred” message.
  • Tests

    • Updated and simplified storage backend RFC0019 scenarios to match the new configuration flow.

…ection

Add `StoreConfig { Local(PathBuf) | S3(S3Config) }` + `open()` to
ourios-parquet (next to Store/S3Config), and resolve it in ourios-server from
`OURIOS_STORAGE_BACKEND` (local default | s3) + `OURIOS_BUCKET_ROOT` /
`OURIOS_S3_{BUCKET,ENDPOINT,REGION,PREFIX}`. Credentials are never read here —
they come from the AWS chain inside `StoreConfig::open` (RFC 0019 §3.4), so a
resolution error names only the missing key, never a secret.

ServerConfig now carries `store: StoreConfig` instead of a bare `bucket_root`.
The receiver/compactor/querier still address the store as a local path, so
`main()` extracts the path for the local backend and fails fast on s3 with a
clear "role wiring lands in a later slice" message — the S3 data path arrives
when the querier/compactor are migrated onto `Store` (RFC0019.3/.4, §3.3).

Greens the config-layer scenarios as unit tests in main.rs (private fns):
RFC0019.1 (selection + fail-fast), .6 (no secret leakage), .7 (local default
unchanged). The S3 server-level scenarios (.2/.3/.4/.5) stay red stubs. Adds
PartialEq/Eq to S3Config + StoreConfig so ServerConfig keeps its derive.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@jensholdgaard
jensholdgaard requested a review from Copilot June 23, 2026 09:21
@coderabbitai

coderabbitai Bot commented Jun 23, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@jensholdgaard, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 22 minutes and 49 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits.

🚦 How do rate limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate.

For paid Pro and Pro+ PR reviews, CodeRabbit uses rolling per-developer review limits. Reviews become available again as older review attempts age out of the rolling limit window.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: a64f1582-35d9-44f5-9aae-497f173b3671

📥 Commits

Reviewing files that changed from the base of the PR and between d839216 and 9dab8a9.

📒 Files selected for processing (1)
  • crates/ourios-server/src/main.rs
📝 Walkthrough

Walkthrough

Introduces a StoreConfig enum (Local(PathBuf) / S3(S3Config)) in ourios-parquet with an open() dispatch method. ourios-server's ServerConfig replaces bucket_root: PathBuf with store: StoreConfig, resolved from env via a new build_store_config helper. An S3 guard in main exits early; local wiring extracts bucket_root for existing receiver/querier/compactor paths. RFC0019 tests move from the integration file into unit tests in main.rs.

Changes

StoreConfig backend abstraction and server wiring

Layer / File(s) Summary
StoreConfig enum and S3Config equality traits
crates/ourios-parquet/src/store.rs, crates/ourios-parquet/src/lib.rs, crates/ourios-server/src/main.rs
S3Config gains PartialEq and Eq. A new StoreConfig public enum with Local(PathBuf) and S3(S3Config) variants and an open() method is added and re-exported. ServerConfig's bucket_root: PathBuf field is replaced with store: StoreConfig.
build_store_config and config_from_env
crates/ourios-server/src/main.rs
config_from_env reads OURIOS_STORAGE_BACKEND and delegates to a new build_store_config helper that validates env vars for local (OURIOS_BUCKET_ROOT) and s3 (OURIOS_S3_BUCKET plus optional endpoint/region/prefix) variants, failing on unknown backends or missing required vars.
build_config signature and main S3 guard
crates/ourios-server/src/main.rs
build_config accepts store: StoreConfig directly. In main, StoreConfig::S3 causes an immediate explicit error exit; for Local, bucket_root is extracted and passed into receiver, querier, and compactor wiring.
RFC0019 unit tests and integration test cleanup
crates/ourios-server/src/main.rs, crates/ourios-server/tests/rfc0019_storage_backend.rs
Unit tests updated to use StoreConfig::Local fixture. New RFC0019 scenarios cover local defaulting, explicit local, S3 with optional addressing, missing/unknown backend failures, and credential non-leakage. Scenarios .1/.6/.7 removed from the integration test file as they are now unit-tested.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • jensholdgaard/ourios#230: Establishes the ourios-parquet store-module skeleton introducing Store and S3Config, which this PR extends with StoreConfig and equality traits.
  • jensholdgaard/ourios#235: Implements Store::s3(S3Config) in crates/ourios-parquet/src/store.rs, the exact method that StoreConfig::open() dispatches to for the S3 variant.
  • jensholdgaard/ourios#245: Wires bucket_root into receiver::ReceiverConfig in crates/ourios-server/src/main.rs, the same receiver wiring this PR updates to extract bucket_root from StoreConfig::Local.
  • jensholdgaard/ourios#287: Adds the RFC 0019 specification for storage-backend selection, which this PR implements via StoreConfig wiring, env-driven backend resolution, and RFC0019-focused unit test coverage.

Poem

🐇 Hopping through config files with glee,
A StoreConfig enum — local or S3!
build_store_config sniffs each env var,
S3 still deferred (not yet gone far).
bucket_root extracted, wiring stays neat,
The rabbit says: "RFC0019, complete!" 🥕

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: introducing StoreConfig and backend selection for RFC 0019 green slice 1, with specific scenario numbers referenced.
Description check ✅ Passed The description comprehensively covers the what/how/tests with RFC context, implementation details, and next steps, though it omits explicit checklist items.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch rfc0019-green-storeconfig

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Implements RFC 0019 “green slice 1” by introducing a StoreConfig seam (local vs S3) and wiring ourios-server to resolve the selected backend from OURIOS_* env vars, while intentionally failing fast at runtime if s3 is selected (until later slices migrate roles onto Store).

Changes:

  • Added StoreConfig { Local(PathBuf) | S3(S3Config) } to ourios-parquet and re-exported it.
  • Updated ourios-server config resolution to build and carry StoreConfig, plus added unit tests for RFC0019 scenarios .1/.6/.7.
  • Adjusted RFC0019 integration test stubs to keep only the remaining server-level ignored scenarios .2.5.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
crates/ourios-server/tests/rfc0019_storage_backend.rs Updates RFC0019 server-level scenario stubs; removes scenarios now covered by unit tests.
crates/ourios-server/src/main.rs Adds backend selection/config parsing into StoreConfig, adds fail-fast for S3 runtime, and adds unit tests for RFC0019 .1/.6/.7.
crates/ourios-parquet/src/store.rs Introduces StoreConfig and adds PartialEq/Eq to S3Config to support comparisons/tests.
crates/ourios-parquet/src/lib.rs Re-exports StoreConfig from the store module.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread crates/ourios-server/src/main.rs Outdated
Comment thread crates/ourios-server/src/main.rs Outdated
Comment thread crates/ourios-parquet/src/store.rs
)

- build_store_config trims OURIOS_STORAGE_BACKEND and treats empty as unset,
  so " s3 " selects S3 and a blank value falls back to local (not "unknown").
  Covered by the RFC0019.1 unit test.
- Tighten the secret-hygiene rustdoc: an error for a *missing required* value
  names only the key; other errors (unknown backend) may echo the non-secret
  value for diagnosability (matches RFC 0019 §3.4).
- Document StoreConfig's deliberate exhaustiveness + why the #[non_exhaustive]
  semver tradeoff doesn't apply (all crates are publish=false / internal).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

Comment thread crates/ourios-server/src/main.rs
)

build_store_config now reports an unset bucket root ("must be set") distinctly
from a present-but-empty one ("must not be empty") for clearer operator
diagnostics, restoring the two-message split the refactor had merged. RFC0019.1
asserts both messages.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants