Skip to content

refactor(log-ingestor)!: Rename ingestion API routes to the /s3/* scheme.#2261

Open
jonathan-imanu wants to merge 1 commit intoy-scope:mainfrom
jonathan-imanu:refactor/route-naming
Open

refactor(log-ingestor)!: Rename ingestion API routes to the /s3/* scheme.#2261
jonathan-imanu wants to merge 1 commit intoy-scope:mainfrom
jonathan-imanu:refactor/route-naming

Conversation

@jonathan-imanu
Copy link
Copy Markdown
Contributor

@jonathan-imanu jonathan-imanu commented May 7, 2026

Description

Rename log-ingestor API routes according to the proposal outlined in #2166

Checklist

  • The PR satisfies the contribution guidelines.
  • This is a breaking change and that has been indicated in the PR title, OR this isn't a
    breaking change.
  • Necessary docs have been updated, OR no docs need to be updated.

Validation performed

  • Workflows

Summary by CodeRabbit

  • Refactor
    • Log ingestion API endpoint paths have been updated and reorganized. The S3 scanner creation, SQS listener creation, and job termination endpoints are now available at new paths. Any systems currently integrating with these operations should update their API calls to use the new endpoint paths.

@jonathan-imanu jonathan-imanu requested a review from a team as a code owner May 7, 2026 23:22
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 7, 2026

Review Change Stack

Walkthrough

The pull request updates three API endpoint paths in the log-ingestor routing configuration. The S3 scanner creation, SQS listener creation, and job termination endpoints are moved to a unified /s3 namespace with slash-delimited path segments, replacing inconsistent underscore-based and mixed-prefix naming conventions. Handler implementations and error handling remain unchanged.

Changes

API Route Path Consolidation

Layer / File(s) Summary
API Route Path Updates
components/log-ingestor/src/routes.rs
Three ingestion-job endpoint paths updated in utoipa annotations: POST /s3_scannerPOST /s3/scanner, POST /sqs_listenerPOST /s3/listener, POST /job/{job_id}/terminatePOST /s3/{job_id}/terminate. Handler implementations and request/response types remain unchanged.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.
Title check ✅ Passed The title clearly and specifically describes the main change: renaming ingestion API routes to follow the /s3/* naming scheme, which directly aligns with the file-level changes shown in the raw summary.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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 and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 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.

Inline comments:
In `@components/log-ingestor/src/routes.rs`:
- Line 248: The route path currently uses a type-specific prefix
"/s3/{job_id}/terminate" which wrongly ties a type-agnostic operation to S3;
change the route to a resource-oriented path such as "/jobs/{job_id}/terminate"
(or "/job/{job_id}/terminate" if that matches existing conventions) and update
the route registration so the handler shutdown_and_remove_job_instance remains
unchanged and continues to accept an IngestionJobId; ensure any doc/comments and
tests referencing the old "/s3/…" path are updated accordingly.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 8df16e85-c681-4a16-a12c-b1ae2ac09c4e

📥 Commits

Reviewing files that changed from the base of the PR and between aeb077c and ed0118a.

⛔ Files ignored due to path filters (1)
  • docs/src/_static/generated/log-ingestor-openapi.json is excluded by !**/generated/**
📒 Files selected for processing (1)
  • components/log-ingestor/src/routes.rs

#[utoipa::path(
post,
path = "/job/{job_id}/terminate",
path = "/s3/{job_id}/terminate",
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.

🧹 Nitpick | 🔵 Trivial | ⚡ Quick win

/s3/{job_id}/terminate conflates job-type namespace with a type-agnostic operation.

job_id carries no type information (it is just an opaque ID). Placing the terminate operation under the /s3/ prefix implies it is specific to S3 jobs. If a non-S3 ingestion job type is added later, a separate termination path would be required, or the /s3/ prefix would have to be extended in an ad-hoc way.

A resource-oriented path like /jobs/{job_id}/terminate or simply the original /job/{job_id}/terminate keeps the termination operation decoupled from job type, matching the semantics of the handler (shutdown_and_remove_job_instance accepts any IngestionJobId). Worth confirming whether issue #2166 explicitly mandates this path shape or whether a generic /jobs/{job_id}/terminate also satisfies the scheme.

🤖 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 `@components/log-ingestor/src/routes.rs` at line 248, The route path currently
uses a type-specific prefix "/s3/{job_id}/terminate" which wrongly ties a
type-agnostic operation to S3; change the route to a resource-oriented path such
as "/jobs/{job_id}/terminate" (or "/job/{job_id}/terminate" if that matches
existing conventions) and update the route registration so the handler
shutdown_and_remove_job_instance remains unchanged and continues to accept an
IngestionJobId; ensure any doc/comments and tests referencing the old "/s3/…"
path are updated accordingly.

@jonathan-imanu jonathan-imanu changed the title refactor(log-ingestor): Rename ingestion API routes to the /s3/* scheme. refactor(log-ingestor)!: Rename ingestion API routes to the /s3/* scheme. May 7, 2026
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.

1 participant