-
Notifications
You must be signed in to change notification settings - Fork 1
feat(go): add simplestorage package for high-level interactions #53
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Not totally sure about the API yet, but I think this will be fine? Signed-off-by: Xe Iaso <xe@tigrisdata.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds a new simplestorage package that provides a simplified, high-level API for interacting with Tigris storage. It also refactors endpoint constants to be shared across packages and renames "Snapshottable" to "SnapshotEnabled" for better clarity.
Changes:
- Introduced
go/simplestoragepackage with Client, Object types and CRUD operations (Get, Put, Delete, List) - Extracted endpoint constants (GlobalEndpoint, FlyEndpoint) in
go/storage.gofor reuse - Added new option functions (WithEndpoint, WithPathStyle) to the storage package
- Renamed
CreateSnapshottableBuckettoCreateSnapshotEnabledBucketacross codebase and documentation
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 21 comments.
Show a summary per file
| File | Description |
|---|---|
| go/storage.go | Extracted endpoint constants and added WithEndpoint and WithPathStyle option functions |
| go/simplestorage/options.go | New file defining Options type and configuration functions for simplestorage client |
| go/simplestorage/client.go | New file implementing simplified storage client with CRUD operations and helper functions |
| go/example_test.go | Updated function name from CreateSnapshottableBucket to CreateSnapshotEnabledBucket |
| go/client.go | Renamed CreateSnapshottableBucket method to CreateSnapshotEnabledBucket |
| go/README.md | Updated documentation to reflect CreateSnapshotEnabledBucket naming change |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The low-level storage client now defaults to reading credentials from TIGRIS_STORAGE_ACCESS_KEY_ID and TIGRIS_STORAGE_SECRET_ACCESS_KEY environment variables, matching the behavior of the simplestorage package. Assisted-by: GLM 4.7 via Claude Code
Use time.Time{} instead of time.Unix(1, 1) as the default zero value
for LastModified fields.
Assisted-by: GLM 4.7 via Claude Code
Signed-off-by: Xe Iaso <xe@tigrisdata.com>
Add validation to ensure BucketName is non-empty when creating a client. If no bucket is provided via TIGRIS_STORAGE_BUCKET env var or WithBucket option, return ErrNoBucketName with a clear error message. Also add table-driven tests using errors.Is to verify the validation. Assisted-by: GLM 4.7 via Claude Code
Signed-off-by: Xe Iaso <xe@tigrisdata.com>
There was a problem hiding this comment.
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 7 out of 7 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func WithBucket(bucketName string) Option { | ||
| return func(o *Options) { | ||
| o.BucketName = bucketName | ||
| } | ||
| } | ||
|
|
||
| // WithFlyEndpoint lets you connect to Tigris' fly.io optimized endpoint. | ||
| // | ||
| // If you are deployed to fly.io, this zero-rates your traffic to Tigris. | ||
| // | ||
| // If you are not deployed to fly.io, please use WithGlobalEndpoint instead. | ||
| func WithFlyEndpoint() Option { | ||
| return func(o *Options) { | ||
| o.BaseEndpoint = storage.FlyEndpoint | ||
| } | ||
| } | ||
|
|
||
| // WithGlobalEndpoint lets you connect to Tigris' globally available endpoint. | ||
| // | ||
| // If you are deployed to fly.io, please use WithFlyEndpoint instead. | ||
| func WithGlobalEndpoint() Option { | ||
| return func(o *Options) { | ||
| o.BaseEndpoint = storage.GlobalEndpoint | ||
| } | ||
| } | ||
|
|
||
| // WithEndpoint sets a custom endpoint for connecting to Tigris. | ||
| // | ||
| // This allows you to connect to a custom Tigris endpoint instead of the default | ||
| // global endpoint. Use this for: | ||
| // - Using a custom proxy or gateway | ||
| // - Testing against local development endpoints | ||
| // | ||
| // For most use cases, consider using WithGlobalEndpoint or WithFlyEndpoint instead. | ||
| func WithEndpoint(endpoint string) Option { | ||
| return func(o *Options) { | ||
| o.BaseEndpoint = endpoint | ||
| } | ||
| } | ||
|
|
||
| // WithRegion lets you statically specify a region for interacting with Tigris. | ||
| // | ||
| // You will almost certainly never need this. This is here for development usecases where the default region is not "auto". | ||
| func WithRegion(region string) Option { | ||
| return func(o *Options) { | ||
| o.Region = region | ||
| } | ||
| } | ||
|
|
||
| // WithPathStyle configures whether to use path-style addressing for S3 requests. | ||
| // | ||
| // By default, Tigris uses virtual-hosted-style addressing (e.g., https://bucket.t3.storage.dev). | ||
| // Path-style addressing (e.g., https://t3.storage.dev/bucket) may be needed for: | ||
| // - Compatibility with older S3 clients that don't support virtual-hosted-style | ||
| // - Working through certain proxies or load balancers that don't support virtual-hosted-style | ||
| // - Local development environments with custom DNS setups | ||
| // | ||
| // Enable this only if you encounter issues with the default virtual-hosted-style addressing. | ||
| func WithPathStyle(enabled bool) Option { | ||
| return func(o *Options) { | ||
| o.UsePathStyle = enabled | ||
| } | ||
| } | ||
|
|
||
| // WithAccessKeypair lets you specify a custom access key and secret access key for interfacing with Tigris. | ||
| // | ||
| // This is useful when you need to load environment variables from somewhere other than the default AWS configuration path. | ||
| func WithAccessKeypair(accessKeyID, secretAccessKey string) Option { | ||
| return func(o *Options) { | ||
| o.AccessKeyID = accessKeyID | ||
| o.SecretAccessKey = secretAccessKey | ||
| } | ||
| } |
Copilot
AI
Jan 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The option functions in this file (WithBucket, WithFlyEndpoint, WithGlobalEndpoint, WithEndpoint, WithRegion, WithPathStyle, WithAccessKeypair) lack test coverage. Since the repository has comprehensive tests for similar option functions in the storage package (storage_test.go), these functions should also have corresponding tests to maintain consistent test coverage across the codebase.
| // of the environment variable `TIGRIS_STORAGE_ACCESS_KEY_ID`. | ||
| AccessKeyID string | ||
|
|
||
| // The access key ID of the Tigris keypair the Client should use. Defaults to the contents |
Copilot
AI
Jan 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation comment incorrectly describes this field as "The access key ID" when it should describe it as "The secret access key" since this is the SecretAccessKey field, not AccessKeyID.
| // The access key ID of the Tigris keypair the Client should use. Defaults to the contents | |
| // The secret access key of the Tigris keypair the Client should use. Defaults to the contents |
| obj.Etag = *resp.ETag | ||
| obj.Version = *resp.VersionId |
Copilot
AI
Jan 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential nil pointer dereference. The code directly dereferences resp.ETag and resp.VersionId without checking if they are nil. Consider using the lower helper function (similar to how it's used in the Get method) to safely handle potentially nil values. For example: obj.Etag = lower(resp.ETag, "") and obj.Version = lower(resp.VersionId, "").
| obj.Etag = *resp.ETag | |
| obj.Version = *resp.VersionId | |
| obj.Etag = lower(resp.ETag, "") | |
| obj.Version = lower(resp.VersionId, "") |
| // Get fetches the contents of an object and its metadata from Tigris. | ||
| func (c *Client) Get(ctx context.Context, key string, opts ...ClientOption) (*Object, error) { | ||
| o := new(ClientOptions).defaults(c.options) | ||
|
|
||
| for _, doer := range opts { | ||
| doer(&o) | ||
| } | ||
|
|
||
| resp, err := c.cli.GetObject( | ||
| ctx, | ||
| &s3.GetObjectInput{ | ||
| Bucket: aws.String(o.BucketName), | ||
| Key: aws.String(key), | ||
| }, | ||
| o.S3Options..., | ||
| ) | ||
|
|
||
| if err != nil { | ||
| return nil, fmt.Errorf("simplestorage: can't get %s/%s: %v", o.BucketName, key, err) | ||
| } | ||
|
|
||
| return &Object{ | ||
| Bucket: o.BucketName, | ||
| Key: key, | ||
| ContentType: lower(resp.ContentType, "application/octet-stream"), | ||
| Etag: lower(resp.ETag, ""), | ||
| Size: lower(resp.ContentLength, 0), | ||
| Version: lower(resp.VersionId, ""), | ||
| LastModified: lower(resp.LastModified, time.Time{}), | ||
| Body: resp.Body, | ||
| }, nil | ||
| } | ||
|
|
||
| // Put puts the contents of an object into Tigris. | ||
| func (c *Client) Put(ctx context.Context, obj *Object, opts ...ClientOption) (*Object, error) { | ||
| o := new(ClientOptions).defaults(c.options) | ||
|
|
||
| for _, doer := range opts { | ||
| doer(&o) | ||
| } | ||
|
|
||
| resp, err := c.cli.PutObject( | ||
| ctx, | ||
| &s3.PutObjectInput{ | ||
| Bucket: aws.String(o.BucketName), | ||
| Key: aws.String(obj.Key), | ||
| Body: obj.Body, | ||
| ContentType: raise(obj.ContentType), | ||
| ContentLength: raise(obj.Size), | ||
| }, | ||
| o.S3Options..., | ||
| ) | ||
|
|
||
| if err != nil { | ||
| return nil, fmt.Errorf("simplestorage: can't put %s/%s: %v", o.BucketName, obj.Key, err) | ||
| } | ||
|
|
||
| obj.Bucket = o.BucketName | ||
| obj.Etag = *resp.ETag | ||
| obj.Version = *resp.VersionId | ||
|
|
||
| return obj, nil | ||
| } | ||
|
|
||
| // Delete removes an object from Tigris. | ||
| func (c *Client) Delete(ctx context.Context, key string, opts ...ClientOption) error { | ||
| o := new(ClientOptions).defaults(c.options) | ||
|
|
||
| for _, doer := range opts { | ||
| doer(&o) | ||
| } | ||
|
|
||
| if _, err := c.cli.DeleteObject( | ||
| ctx, | ||
| &s3.DeleteObjectInput{ | ||
| Bucket: aws.String(o.BucketName), | ||
| Key: aws.String(key), | ||
| }, | ||
| o.S3Options..., | ||
| ); err != nil { | ||
| return fmt.Errorf("simplestorage: can't delete %s/%s: %v", o.BucketName, key, err) | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| // List returns a list of objects matching a key prefix. | ||
| func (c *Client) List(ctx context.Context, prefix string, opts ...ClientOption) ([]Object, error) { | ||
| o := new(ClientOptions).defaults(c.options) | ||
|
|
||
| for _, doer := range opts { | ||
| doer(&o) | ||
| } | ||
|
|
||
| resp, err := c.cli.ListObjectsV2( | ||
| ctx, | ||
| &s3.ListObjectsV2Input{ | ||
| Bucket: aws.String(o.BucketName), | ||
| Prefix: aws.String(prefix), | ||
|
|
||
| MaxKeys: o.MaxKeys, | ||
| StartAfter: o.StartAfter, | ||
| }, | ||
| o.S3Options..., | ||
| ) | ||
|
|
||
| if err != nil { | ||
| return nil, fmt.Errorf("simplestorage: can't list %s/%s: %v", o.BucketName, prefix, err) | ||
| } | ||
|
|
||
| var result []Object | ||
|
|
||
| for _, obj := range resp.Contents { | ||
| result = append(result, Object{ | ||
| Bucket: o.BucketName, | ||
| Key: *obj.Key, | ||
| Etag: lower(obj.ETag, ""), | ||
| Size: lower(obj.Size, 0), | ||
| LastModified: lower(obj.LastModified, time.Time{}), | ||
| }) | ||
| } | ||
|
|
||
| return result, nil | ||
| } |
Copilot
AI
Jan 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Get, Put, Delete, and List methods lack test coverage. Since the repository has comprehensive testing for other packages (as seen in storage_test.go), these new public methods should also have tests to ensure correctness and maintain the project's testing standards.
| for _, obj := range resp.Contents { | ||
| result = append(result, Object{ | ||
| Bucket: o.BucketName, | ||
| Key: *obj.Key, |
Copilot
AI
Jan 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential nil pointer dereference. The code directly dereferences obj.Key without checking if it's nil. Consider using the lower helper function to safely handle potentially nil values: Key: lower(obj.Key, "").
| Key: *obj.Key, | |
| Key: lower(obj.Key, ""), |
| BaseEndpoint: "https://t3.storage.dev", | ||
| Region: "auto", | ||
| UsePathStyle: false, | ||
| BaseEndpoint: "https://t3.storage.dev", |
Copilot
AI
Jan 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The BaseEndpoint is hardcoded as "https://t3.storage.dev" instead of using the GlobalEndpoint constant defined on line 18. For consistency and maintainability, this should use GlobalEndpoint instead of the hardcoded string.
| BaseEndpoint: "https://t3.storage.dev", | |
| BaseEndpoint: GlobalEndpoint, |
|
🎉 This PR is included in version 1.0.10 🎉 The release is available on npm package (@latest dist-tag) Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.1.4 🎉 The release is available on npm package (@latest dist-tag) Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 2.11.0 🎉 The release is available on npm package (@latest dist-tag) Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.1.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Not totally sure about the API yet, but I think this will be fine?