Skip to content

feat: Add S3 remote provider for S3 file storage to LLS midst…#137

Merged
Elbehery merged 1 commit intomainfrom
RHAIENG-2128
Nov 26, 2025
Merged

feat: Add S3 remote provider for S3 file storage to LLS midst…#137
Elbehery merged 1 commit intomainfrom
RHAIENG-2128

Conversation

@Elbehery
Copy link
Copy Markdown
Collaborator

@Elbehery Elbehery commented Nov 25, 2025

This MR enables S3-compatible storage backend support for the files API. The boto3 dependency is automatically resolved by the build system.

Signed-off-by: Mustafa Elbehery melbeher@redhat.com

Closes https://issues.redhat.com/browse/RHAIENG-2128

Summary by CodeRabbit

  • New Features
    • Added support for remote S3 storage as a new distribution provider option.

✏️ Tip: You can customize this high-level summary in your review settings.

…ream image.

This MR enables S3-compatible storage backend support for the files API.
The boto3 dependency is automatically resolved by the build system.

Signed-off-by: Mustafa Elbehery <melbeher@redhat.com>

Closes https://issues.redhat.com/browse/RHAIENG-2128
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Nov 25, 2025

Walkthrough

A new S3 remote provider entry is added to the build configuration under distribution_spec.files. The change introduces a single provider type specification without modifying existing providers or infrastructure code.

Changes

Cohort / File(s) Change Summary
S3 Provider Configuration
distribution/build.yaml
Adds new provider entry with provider_type: remote::s3 under distribution_spec.files

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

  • Single configuration-only change with no logic or code modifications
  • Straightforward provider entry addition following existing structure

Poem

🐰 A carrot to the cloud so high,
S3 storage in the sky,
One line added, clean and bright,
Distribution builds with delight!

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main change: adding an S3 remote provider for file storage, which aligns with the pull request objective of enabling S3-compatible storage backend support.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch RHAIENG-2128

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c20784c and 27b8861.

📒 Files selected for processing (1)
  • distribution/build.yaml (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: build-test-push (linux/amd64)
  • GitHub Check: Summary
🔇 Additional comments (1)
distribution/build.yaml (1)

44-45: The S3 provider configuration is correct and properly supported.

The change is verified:

  1. boto3 dependency: boto3==1.35.88 is explicitly pinned in distribution/build.py line 27, ensuring it will be included in the build output.

  2. S3 provider pattern: The remote::s3 entry correctly follows the configuration pattern of other remote providers like remote::bedrock, remote::vllm, and remote::azure—all of which omit explicit module specifications and rely on dynamic provider loading by the llama-stack library.

  3. YAML syntax: Verified valid.

No additional configuration files or changes are required.

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

  • Provide your own instructions using the high_level_summary_instructions setting.
  • Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
  • Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

  1. 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
  2. 📓 References — List relevant issues, discussions, documentation, or related PRs.
  3. 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
  4. 📊 Contributor Summary — Include a Markdown table showing contributions:
    | Contributor | Lines Added | Lines Removed | Files Changed |
  5. ✔️ Additional Notes — Add any extra reviewer context.
    Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.


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.

@Elbehery Elbehery changed the title RHAIENG-2128: Add S3 remote provider for S3 file storage to LLS midst… feat: Add S3 remote provider for S3 file storage to LLS midst… Nov 25, 2025
@Elbehery Elbehery requested review from leseb and nathan-weinberg and removed request for nathan-weinberg November 25, 2025 16:16
@Elbehery
Copy link
Copy Markdown
Collaborator Author

cc @nathan-weinberg

@nathan-weinberg
Copy link
Copy Markdown
Collaborator

Why is this a separate PR from #138?

@Elbehery
Copy link
Copy Markdown
Collaborator Author

Why is this a separate PR from #138?

followed the same way as in downstream, provider first then config

@nathan-weinberg
Copy link
Copy Markdown
Collaborator

We've never done that in the midstream, why now?

@Elbehery
Copy link
Copy Markdown
Collaborator Author

We've never done that in the midstream, why now?

as I said above, I followed that pattern as in downstream

I am confident that this answer your question

@nathan-weinberg
Copy link
Copy Markdown
Collaborator

Better to have it as one PR since CI will be able to test it properly - will approve for now but please return to doing these as one PR in the midstream in the future

@Elbehery
Copy link
Copy Markdown
Collaborator Author

Better to have it as one PR since CI will be able to test it properly - will approve for now but please return to doing these as one PR in the midstream in the future

Yes u right, sorry for the oversight this time 👍🏽 🙏🏽

@Elbehery
Copy link
Copy Markdown
Collaborator Author

cc @skamenan7 ptal

@Elbehery Elbehery merged commit 437e5af into main Nov 26, 2025
5 of 7 checks passed
@Elbehery Elbehery deleted the RHAIENG-2128 branch November 26, 2025 15:12
@nathan-weinberg
Copy link
Copy Markdown
Collaborator

don't 👏 merge 👏 manually 👏

@Elbehery
Copy link
Copy Markdown
Collaborator Author

don't 👏 merge 👏 manually 👏

sry I forgot :'(

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.

3 participants