feat: custom s3 url #14
Conversation
Signed-off-by: Lukas Hoehl <[email protected]>
There was a problem hiding this comment.
This change seems fine, but we should also update the documentation in help.go, and I have a few other small comments inline.
Can you please also update the commit title and description to provide more context for what this is doing? We also don't use the conventional-commits type tags like "feat" for PR titles.
A suggestion for example:
cmd/go-cache-plugin: add support for a custom S3 endpoint URL
This allows use of other S3-compatible implementations for cache storage.
or words to that effect.
| @@ -26,6 +26,7 @@ var flags struct { | |||
| CacheDir string `flag:"cache-dir,default=$GOCACHE_DIR,Local cache directory (required)"` | |||
| S3Bucket string `flag:"bucket,default=$GOCACHE_S3_BUCKET,S3 bucket name (required)"` | |||
| S3Region string `flag:"region,default=$GOCACHE_S3_REGION,S3 region"` | |||
| S3URL string `flag:"s3-url,default=$GOCACHE_S3_URL,S3 URL"` | |||
There was a problem hiding this comment.
I think this flag needs a more descriptive name. Perhaps:
| S3URL string `flag:"s3-url,default=$GOCACHE_S3_URL,S3 URL"` | |
| S3Endpoint string `flag:"s3-endpoint-url,default=$GOCACHE_S3_ENDPOINT_URL,S3 custom endpoint URL (if unset, use AWS default)"` |
In particular: Make the name reflect that this is the endpoint URL, and note the meaning of the default.
| @@ -62,10 +62,19 @@ func initCacheServer(env *command.Env) (*gocache.Server, *s3util.Client, error) | |||
|
|
|||
| vprintf("local cache directory: %s", flags.CacheDir) | |||
| vprintf("S3 cache bucket %q (%s)", flags.S3Bucket, region) | |||
| client := &s3util.Client{ | |||
| Client: s3.NewFromConfig(cfg, func(o *s3.Options) { | |||
| s3opts := []func(*s3.Options){ | |||
There was a problem hiding this comment.
Naming nit: Mixed case, please. I would just call this opts since there are no other options in scope here.
| }, | ||
| } | ||
| if flags.S3URL != "" { | ||
| vprintf("S3 URL: %s", flags.S3URL) |
There was a problem hiding this comment.
| vprintf("S3 URL: %s", flags.S3URL) | |
| vprintf("S3 endpoint URL: %s", flags.S3Endpoint) |
| if flags.S3URL != "" { | ||
| vprintf("S3 URL: %s", flags.S3URL) | ||
| s3opts = append(s3opts, func(o *s3.Options) { | ||
| o.BaseEndpoint = &flags.S3URL |
There was a problem hiding this comment.
I suggest making this
| o.BaseEndpoint = &flags.S3URL | |
| o.BaseEndpoint = aws.String(flags.S3URL) |
The meaning is the same, but makes the intent clearer.
There was a problem hiding this comment.
Oh, actually maybe a better way to do this is to add config.WithBaseEndpoint to the LoadDefaultConfig call, instead of here?
Allows to use custom endpoints that implement s3 API.