refactor: move S3 configuration to its own package

[kalbasit]

No description provided.

[gemini-code-assist]

Summary of Changes

Hello @kalbasit, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly refactors the S3 storage configuration by extracting the Config struct, its validation, and associated utility functions into a new, dedicated pkg/s3 package. This change improves modularity and organization, centralizing S3-specific configuration logic and making it more reusable across different parts of the application. The existing pkg/storage/s3 package has been updated to consume this new configuration package, streamlining its dependencies and reducing code duplication.

Highlights

• New S3 Configuration Package: A new package, pkg/s3, has been introduced to centralize the S3 configuration struct (Config), its validation logic, and related helper functions (ValidateConfig, GetEndpointWithoutScheme, IsHTTPS).
• Refactored S3 Storage Implementation: The pkg/storage/s3 package has been updated to remove its local Config struct, validation, and helper functions. It now imports and utilizes the s3.Config and associated utilities from the newly created pkg/s3 package.
• Improved MinIO Client Initialization: The Transport field from the S3 configuration is now correctly passed to the MinIO client initialization, allowing for custom HTTP transport configurations.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[kalbasit]

• style: apply nix fmt to documentation and project files #671
• feat: add CLI and Helm configuration for CDC and S3 #670
• feat: integrate CDC with cache and implement chunk GC #669
• feat: add S3 storage support for chunks #668
• refactor: extract S3 test helpers for reuse #676
• refactor: move S3 configuration to its own package #675 👈 (View in Graphite)
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[gemini-code-assist]

Code Review

This pull request refactors the S3 configuration into its own pkg/s3 package, which is a good step for modularity. However, I've found a few issues that need to be addressed:

• The refactoring breaks existing tests in pkg/storage/s3/s3_test.go because the tested functions have been moved or removed. The tests need to be updated or moved to the new package.
• The new GetEndpointWithoutScheme function in pkg/s3/config.go is buggy as it doesn't handle URL paths correctly.
• The endpoint validation can be made more robust by using the net/url package.

Please see my detailed comments for suggestions on how to address these points.

I am having trouble creating individual review comments. Click here to see my feedback.

pkg/storage/s3/s3.go (558-582)

By removing ValidateConfig and the logic for GetEndpointWithoutScheme and IsHTTPS from this package, you are breaking the existing tests in pkg/storage/s3/s3_test.go. The tests for this functionality should be moved to a new test file in the pkg/s3 package (e.g., pkg/s3/config_test.go) and the existing tests in s3_test.go should be updated to call the new pkg/s3 functions or removed if they are now redundant.

References

1. Risky behavioral changes should not be included in a refactoring pull request and should be addressed separately. Breaking existing tests during a refactoring indicates a potential risky change or an incomplete refactoring that needs careful attention.

pkg/s3/config.go (75-80)

The current implementation of GetEndpointWithoutScheme only removes the scheme prefix. If the endpoint URL contains a path (e.g., https://my-s3.com/some/path), the path will be incorrectly included in the returned endpoint string. The MinIO client expects only the host and port for the endpoint. Using net/url.Parse and returning u.Host would be more robust and correct. You'll need to add "net/url" to your imports.

func GetEndpointWithoutScheme(endpoint string) string {
	// This function assumes a valid URL with a scheme, as validated by ValidateConfig.
	// We can ignore the error from url.Parse.
	u, _ := url.Parse(endpoint)
	return u.Host
}

pkg/s3/config.go (58)

The current validation for the endpoint scheme using strings.HasPrefix is not fully robust as it can allow malformed URLs (e.g., http:foo). Using net/url.Parse provides more reliable validation of the URL structure and its scheme. This also makes the code's intent clearer. You'll need to add "net/url" to your imports.

if u, err := url.Parse(cfg.Endpoint); err != nil || (u.Scheme != "http" && u.Scheme != "https") {

[kalbasit]

/gemini review

[gemini-code-assist]

Code Review

This pull request is a nice refactoring that moves the S3 configuration into its own dedicated package pkg/s3. This improves modularity and makes the S3 configuration logic more reusable and robust. The new ValidateConfig function with specific error variables and the improved GetEndpointWithoutScheme and IsHTTPS helpers are great additions. The tests have also been correctly moved and updated.

I've found one minor issue regarding some redundant wrapper functions left over after the refactoring. Please see my comment below.

[codecov]

Codecov Report

❌ Patch coverage is 71.42857% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.98%. Comparing base (c7ef2c0) to head (c54d859).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/storage/s3/example.go	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #675      +/-   ##
==========================================
- Coverage   69.51%   67.98%   -1.54%     
==========================================
  Files           2        2              
  Lines         479      456      -23     
==========================================
- Hits          333      310      -23     
  Misses        129      129              
  Partials       17       17              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.