Skip to content

Allow s3 calls to disable multipart upload#2738

Merged
selzoc merged 1 commit into
mainfrom
add-s3-enable-multipart-property
Jun 2, 2026
Merged

Allow s3 calls to disable multipart upload#2738
selzoc merged 1 commit into
mainfrom
add-s3-enable-multipart-property

Conversation

@selzoc
Copy link
Copy Markdown
Member

@selzoc selzoc commented Jun 1, 2026

Following on to #2726

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Jun 1, 2026

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 52dc2cf4-47aa-4343-9187-2f74ac7af57a

📥 Commits

Reviewing files that changed from the base of the PR and between c8902fc and c2ada63.

📒 Files selected for processing (4)
  • jobs/director/spec
  • jobs/director/templates/director.yml.erb
  • src/bosh-director/lib/bosh/director/blobstore/s3cli_blobstore_client.rb
  • src/bosh-director/spec/unit/bosh/director/blobstore/s3cli_blobstore_client_spec.rb

Walkthrough

This PR adds S3 multipart upload configuration to BOSH Director. A new property blobstore.multipart_upload_enabled is declared in the director job spec, then conditionally wired through the director template into the blobstore configuration options. The S3 blobstore client is updated to accept the multipart_upload option and pass it to the s3cli configuration, with test coverage validating the configuration flows through correctly.

Suggested reviewers

  • ragaskar
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description is incomplete, providing only a reference to PR #2726 without addressing any of the required template sections. Add comprehensive information covering: what the change does and why it's needed, contextual links, tests run, release notes summary, breaking change assessment, and team tags.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: adding the ability to disable S3 multipart upload.
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 docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch add-s3-enable-multipart-property

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.

coderabbitai[bot]
coderabbitai Bot previously requested changes Jun 1, 2026
Copy link
Copy Markdown

@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: 2

🤖 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 `@src/bosh-director/lib/bosh/director/blobstore/s3cli_blobstore_client.rb`:
- Line 51: The code uses the wrong options key — it reads
`@options`[:multipart_upload_enabled] but the template sets multipart_upload, so
the value becomes nil and the property is dropped; update the blobstore client
to read the correct key (use `@options`[:multipart_upload]) or ensure the template
writes the _enabled key consistently (i.e., make the code in
src/bosh-director/lib/bosh/director/blobstore/s3cli_blobstore_client.rb
reference `@options`[:multipart_upload] where multipart_upload is assigned) so the
multipart_upload setting is preserved end-to-end.

In
`@src/bosh-director/spec/unit/bosh/director/blobstore/s3cli_blobstore_client_spec.rb`:
- Around line 153-163: The test is passing the wrong option key
(`multipart_upload_enabled`) to described_class.new; change the test to pass
`multipart_upload: false` via the options hash so it matches the director
template contract and the code that writes the config; then keep the assertion
on stored_config_file/JSON parse as-is (expect config['multipart_upload'] to
eq(false)) so the spec verifies the real wiring between the client initializer
(described_class.new) and the stored config.
🪄 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: 957a756f-8558-4b6d-821c-fe5d326ae50c

📥 Commits

Reviewing files that changed from the base of the PR and between e3bf5b9 and c8902fc.

📒 Files selected for processing (4)
  • jobs/director/spec
  • jobs/director/templates/director.yml.erb
  • src/bosh-director/lib/bosh/director/blobstore/s3cli_blobstore_client.rb
  • src/bosh-director/spec/unit/bosh/director/blobstore/s3cli_blobstore_client_spec.rb

Comment thread src/bosh-director/lib/bosh/director/blobstore/s3cli_blobstore_client.rb Outdated
Following on to #2726

ai-assisted=yes
[TNZ-105379]
@selzoc selzoc force-pushed the add-s3-enable-multipart-property branch from c8902fc to c2ada63 Compare June 1, 2026 21:21
@selzoc selzoc merged commit 1e733d5 into main Jun 2, 2026
22 checks passed
@selzoc selzoc deleted the add-s3-enable-multipart-property branch June 2, 2026 20:02
@github-project-automation github-project-automation Bot moved this from Pending Merge | Prioritized to Done in Foundational Infrastructure Working Group Jun 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

2 participants