feat: [AH-2353]: Updated storage layer initiation for default bucket#88
Open
abhinavcode wants to merge 2 commits into
Open
feat: [AH-2353]: Updated storage layer initiation for default bucket#88abhinavcode wants to merge 2 commits into
abhinavcode wants to merge 2 commits into
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
📝 WalkthroughWalkthroughThis pull request adds Google Cloud Storage (GCS) as a new storage backend option. It introduces GCS configuration support, implements the GCS driver with storage operations, adds a configuration helper, and updates the storage initialization wiring to support GCS selection via storage type configuration. Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant Wire as wire.go
participant Config as Config Helper
participant GCSReg as GCS Registration
participant Driver as GCS Driver
participant Client as GCS Client
App->>Wire: Initialize Storage (StorageType: "gcs")
Wire->>Config: GetGCSStorageParameters(config)
Config-->>Wire: GCS Parameters Map
Wire->>GCSReg: Register(ctx)
GCSReg->>Driver: Create Driver Instance
Driver->>Client: Initialize Storage Client
Client-->>Driver: Client Ready
Driver-->>GCSReg: Driver Registered
GCSReg-->>Wire: Registration Complete
Wire-->>App: Storage Initialized
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
registry/app/api/wire.go (1)
98-104: GCS error handling useslog.Error+panic— consider usinglog.Fatalor just returning the error.The
filesystemcase useslog.Fatal(which callsos.Exit), whilegcsands3cases uselog.Error+panic.panicin a provider function is harsh — if the wire injection catches panics differently thanos.Exit, behavior will diverge. Returning(nil, err)would be more idiomatic and let the caller decide how to handle it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@registry/app/api/wire.go` around lines 98 - 104, The gcs case in registry/app/api/wire.go calls gcs.Register(ctx) and then panics on factory.Create error; instead of panic (or using log.Fatal), log the error with the existing logger (including the error/stack) and return the error to the caller (e.g., return nil, err) so behavior matches the idiomatic handling used elsewhere (the filesystem case uses log.Fatal) and lets the caller decide termination; update the error branch after factory.Create in the gcs case to log the error and return (nil, err) rather than calling panic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@registry/app/api/wire.go`:
- Around line 90-112: The switch on c.Registry.Storage.StorageType currently
treats any unknown value as S3; add an explicit "s3aws" case that calls
s3.Register(ctx) and sets d, err = factory.Create(ctx, "s3aws",
config.GetS3StorageParameters(c)) (mirroring the existing S3 logic) and change
the switch default to surface a configuration error instead of initializing S3
(e.g., return or log.Fatalf with a clear message including the invalid
c.Registry.Storage.StorageType); update callers or function signature if needed
so the function returns an error rather than silently panicking or defaulting.
In `@registry/app/driver/gcs/gcs.go`:
- Around line 941-959: CopyObject currently resolves the source key with
d.pathToKey but uses destKey verbatim; make path handling consistent by applying
d.pathToKey to the destination (e.g., use
d.gcs.Bucket(destBucket).Object(d.pathToKey(destKey))) so destination objects
live under the same rootDirectory, and add a pre-check that destBucket is
non-empty (return a clear error if empty) before creating the destination
object; update any related error messages in CopyObject to reference the
resolved destination key.
In `@registry/config/helper.go`:
- Around line 49-53: GetGCSStorageParameters currently only returns the bucket
and omits the rootdirectory (and any other new GCS options), causing all objects
to be written to the bucket root; update GetGCSStorageParameters to read
c.Registry.Storage.GCSStorage.RootDirectory (and any other new fields) and
include them in the returned map (e.g., set "rootdirectory" =>
c.Registry.Storage.GCSStorage.RootDirectory) so the GCS driver receives the same
namespace/isolation params as GetS3StorageParameters and GetFilesystemParams.
In `@types/config.go`:
- Around line 548-552: The GCSStorage struct is missing RootDirectory (and other
driver params) so registry objects would be stored at the bucket root; update
the GCSStorage struct to add at least a RootDirectory string field (matching
envconfig tag pattern like GITNESS_REGISTRY_GCS_ROOT_DIRECTORY) and consider
adding optional fields supported by the driver such as ChunkSize,
MaxConcurrency, KeyFile, and Credentials to mirror S3Storage/FileSystemStorage;
ensure the struct field names and envconfig tags align with how FromParameters
expects keys so FromParameters can map rootdirectory, chunksize, maxconcurrency,
keyfile, and credentials into the GCSStorage config.
---
Nitpick comments:
In `@registry/app/api/wire.go`:
- Around line 98-104: The gcs case in registry/app/api/wire.go calls
gcs.Register(ctx) and then panics on factory.Create error; instead of panic (or
using log.Fatal), log the error with the existing logger (including the
error/stack) and return the error to the caller (e.g., return nil, err) so
behavior matches the idiomatic handling used elsewhere (the filesystem case uses
log.Fatal) and lets the caller decide termination; update the error branch after
factory.Create in the gcs case to log the error and return (nil, err) rather
than calling panic.
Comment on lines
+90
to
112
| switch c.Registry.Storage.StorageType { | ||
| case "filesystem": | ||
| filesystem.Register(ctx) | ||
| d, err = factory.Create(ctx, "filesystem", config.GetFilesystemParams(c)) | ||
| if err != nil { | ||
| log.Fatal().Stack().Err(err).Msgf("") | ||
| panic(err) | ||
| } | ||
| } else { | ||
| case "gcs": | ||
| gcs.Register(ctx) | ||
| d, err = factory.Create(ctx, "gcs", config.GetGCSStorageParameters(c)) | ||
| if err != nil { | ||
| log.Error().Stack().Err(err).Msg("failed to init gcs Blob storage") | ||
| panic(err) | ||
| } | ||
| default: | ||
| s3.Register(ctx) | ||
| d, err = factory.Create(ctx, "s3aws", config.GetS3StorageParameters(c)) | ||
| if err != nil { | ||
| log.Error().Stack().Err(err).Msg("failed to init s3 Blob storage ") | ||
| log.Error().Stack().Err(err).Msg("failed to init s3 Blob storage") | ||
| panic(err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Unknown StorageType values silently fall through to S3.
If someone mistypes the storage type (e.g., "gc" instead of "gcs"), the default branch will quietly initialize S3 storage instead of surfacing a configuration error. Consider adding an explicit "s3aws" case and making default return an error for unrecognized values.
Suggested change
switch c.Registry.Storage.StorageType {
case "filesystem":
filesystem.Register(ctx)
d, err = factory.Create(ctx, "filesystem", config.GetFilesystemParams(c))
if err != nil {
log.Fatal().Stack().Err(err).Msgf("")
panic(err)
}
case "gcs":
gcs.Register(ctx)
d, err = factory.Create(ctx, "gcs", config.GetGCSStorageParameters(c))
if err != nil {
log.Error().Stack().Err(err).Msg("failed to init gcs Blob storage")
panic(err)
}
-default:
+case "s3aws":
s3.Register(ctx)
d, err = factory.Create(ctx, "s3aws", config.GetS3StorageParameters(c))
if err != nil {
log.Error().Stack().Err(err).Msg("failed to init s3 Blob storage")
panic(err)
}
+default:
+ return nil, fmt.Errorf("unsupported registry storage type: %q", c.Registry.Storage.StorageType)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| switch c.Registry.Storage.StorageType { | |
| case "filesystem": | |
| filesystem.Register(ctx) | |
| d, err = factory.Create(ctx, "filesystem", config.GetFilesystemParams(c)) | |
| if err != nil { | |
| log.Fatal().Stack().Err(err).Msgf("") | |
| panic(err) | |
| } | |
| } else { | |
| case "gcs": | |
| gcs.Register(ctx) | |
| d, err = factory.Create(ctx, "gcs", config.GetGCSStorageParameters(c)) | |
| if err != nil { | |
| log.Error().Stack().Err(err).Msg("failed to init gcs Blob storage") | |
| panic(err) | |
| } | |
| default: | |
| s3.Register(ctx) | |
| d, err = factory.Create(ctx, "s3aws", config.GetS3StorageParameters(c)) | |
| if err != nil { | |
| log.Error().Stack().Err(err).Msg("failed to init s3 Blob storage ") | |
| log.Error().Stack().Err(err).Msg("failed to init s3 Blob storage") | |
| panic(err) | |
| } | |
| } | |
| switch c.Registry.Storage.StorageType { | |
| case "filesystem": | |
| filesystem.Register(ctx) | |
| d, err = factory.Create(ctx, "filesystem", config.GetFilesystemParams(c)) | |
| if err != nil { | |
| log.Fatal().Stack().Err(err).Msgf("") | |
| panic(err) | |
| } | |
| case "gcs": | |
| gcs.Register(ctx) | |
| d, err = factory.Create(ctx, "gcs", config.GetGCSStorageParameters(c)) | |
| if err != nil { | |
| log.Error().Stack().Err(err).Msg("failed to init gcs Blob storage") | |
| panic(err) | |
| } | |
| case "s3aws": | |
| s3.Register(ctx) | |
| d, err = factory.Create(ctx, "s3aws", config.GetS3StorageParameters(c)) | |
| if err != nil { | |
| log.Error().Stack().Err(err).Msg("failed to init s3 Blob storage") | |
| panic(err) | |
| } | |
| default: | |
| return nil, fmt.Errorf("unsupported registry storage type: %q", c.Registry.Storage.StorageType) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@registry/app/api/wire.go` around lines 90 - 112, The switch on
c.Registry.Storage.StorageType currently treats any unknown value as S3; add an
explicit "s3aws" case that calls s3.Register(ctx) and sets d, err =
factory.Create(ctx, "s3aws", config.GetS3StorageParameters(c)) (mirroring the
existing S3 logic) and change the switch default to surface a configuration
error instead of initializing S3 (e.g., return or log.Fatalf with a clear
message including the invalid c.Registry.Storage.StorageType); update callers or
function signature if needed so the function returns an error rather than
silently panicking or defaulting.
Comment on lines
+941
to
959
| func (d *driver) CopyObject(ctx context.Context, srcKey, destBucket, destKey string) error { | ||
| src := d.bucket.Object(d.pathToKey(srcKey)) | ||
|
|
||
| dst := d.gcs.Bucket(destBucket).Object(destKey) | ||
|
|
||
| copier := dst.CopierFrom(src) | ||
| copier.ContentType = blobContentType | ||
|
|
||
| _, err := copier.Run(ctx) | ||
| if err != nil { | ||
| var status *googleapi.Error | ||
| if errors.As(err, &status) && status.Code == http.StatusNotFound { | ||
| return storagedriver.PathNotFoundError{Path: srcKey} | ||
| } | ||
| return fmt.Errorf("copy %q to %q/%q: %w", srcKey, destBucket, destKey, err) | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
Asymmetric path handling: srcKey is resolved via pathToKey but destKey is used verbatim.
d.pathToKey(srcKey) prepends rootDirectory to the source, but destKey is passed directly to d.gcs.Bucket(destBucket).Object(destKey) without any root-directory prefix. If the caller isn't aware of this inconsistency, the destination object will end up at an unexpected path. Either document this contract clearly or apply consistent path resolution.
Additionally, consider whether this method should verify that destBucket is non-empty before proceeding, to avoid a confusing GCS error.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@registry/app/driver/gcs/gcs.go` around lines 941 - 959, CopyObject currently
resolves the source key with d.pathToKey but uses destKey verbatim; make path
handling consistent by applying d.pathToKey to the destination (e.g., use
d.gcs.Bucket(destBucket).Object(d.pathToKey(destKey))) so destination objects
live under the same rootDirectory, and add a pre-check that destBucket is
non-empty (return a clear error if empty) before creating the destination
object; update any related error messages in CopyObject to reference the
resolved destination key.
Comment on lines
+49
to
+53
| func GetGCSStorageParameters(c *types.Config) map[string]any { | ||
| gcsProperties := make(map[string]any) | ||
| gcsProperties["bucket"] = c.Registry.Storage.GCSStorage.Bucket | ||
| return gcsProperties | ||
| } |
There was a problem hiding this comment.
Pass rootdirectory (once added to config) for namespace isolation.
This helper should forward the rootdirectory parameter (and any other newly-added GCS config fields) to the driver, matching the pattern used by GetS3StorageParameters and GetFilesystemParams. Currently all registry objects will be written directly under the bucket root.
Suggested change (once config is updated)
func GetGCSStorageParameters(c *types.Config) map[string]any {
gcsProperties := make(map[string]any)
gcsProperties["bucket"] = c.Registry.Storage.GCSStorage.Bucket
+ gcsProperties["rootdirectory"] = c.Registry.Storage.GCSStorage.RootDirectory
return gcsProperties
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@registry/config/helper.go` around lines 49 - 53, GetGCSStorageParameters
currently only returns the bucket and omits the rootdirectory (and any other new
GCS options), causing all objects to be written to the bucket root; update
GetGCSStorageParameters to read c.Registry.Storage.GCSStorage.RootDirectory (and
any other new fields) and include them in the returned map (e.g., set
"rootdirectory" => c.Registry.Storage.GCSStorage.RootDirectory) so the GCS
driver receives the same namespace/isolation params as GetS3StorageParameters
and GetFilesystemParams.
Comment on lines
+548
to
+552
| // GCSStorage defines the configuration for the GCS storage if StorageType is `gcs`. | ||
| // Authentication is handled via workload identity (google.DefaultTokenSource). | ||
| GCSStorage struct { | ||
| Bucket string `envconfig:"GITNESS_REGISTRY_GCS_BUCKET"` | ||
| } |
There was a problem hiding this comment.
GCSStorage config is missing RootDirectory (and other driver parameters).
Both S3Storage and FileSystemStorage expose a RootDirectory field to isolate registry data under a path prefix. The GCS driver's FromParameters also supports rootdirectory, chunksize, maxconcurrency, keyfile, and credentials, but none of these are configurable here. At minimum, RootDirectory should be added for parity and to avoid storing objects at the bucket root with no namespace isolation.
Suggested minimal addition
GCSStorage struct {
- Bucket string `envconfig:"GITNESS_REGISTRY_GCS_BUCKET"`
+ Bucket string `envconfig:"GITNESS_REGISTRY_GCS_BUCKET"`
+ RootDirectory string `envconfig:"GITNESS_REGISTRY_GCS_ROOT_DIRECTORY"`
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // GCSStorage defines the configuration for the GCS storage if StorageType is `gcs`. | |
| // Authentication is handled via workload identity (google.DefaultTokenSource). | |
| GCSStorage struct { | |
| Bucket string `envconfig:"GITNESS_REGISTRY_GCS_BUCKET"` | |
| } | |
| // GCSStorage defines the configuration for the GCS storage if StorageType is `gcs`. | |
| // Authentication is handled via workload identity (google.DefaultTokenSource). | |
| GCSStorage struct { | |
| Bucket string `envconfig:"GITNESS_REGISTRY_GCS_BUCKET"` | |
| RootDirectory string `envconfig:"GITNESS_REGISTRY_GCS_ROOT_DIRECTORY"` | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@types/config.go` around lines 548 - 552, The GCSStorage struct is missing
RootDirectory (and other driver params) so registry objects would be stored at
the bucket root; update the GCSStorage struct to add at least a RootDirectory
string field (matching envconfig tag pattern like
GITNESS_REGISTRY_GCS_ROOT_DIRECTORY) and consider adding optional fields
supported by the driver such as ChunkSize, MaxConcurrency, KeyFile, and
Credentials to mirror S3Storage/FileSystemStorage; ensure the struct field names
and envconfig tags align with how FromParameters expects keys so FromParameters
can map rootdirectory, chunksize, maxconcurrency, keyfile, and credentials into
the GCSStorage config.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements