-
Notifications
You must be signed in to change notification settings - Fork 0
store: add initial S3 store implementation #3
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
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.
LGTM with some minor comments.
) | ||
|
||
const ( | ||
// AccountParam is the AWS access key ID. |
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.
These comments are pointless.
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.
some of these are required by the linter.
Obfuscated = "******" | ||
) | ||
|
||
// ErrMissingParam is returned when required parameters are missing. |
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.
unneeded comment
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.
required by linter.
// candidateConfigs provides a set of candidate configurations for the S3 store. | ||
func (s *s3Store) candidateConfigs() iter.Seq[Store] { | ||
return func(yield func(Store) bool) { | ||
combos := [][]string{ |
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.
why redefine this here and not put it in the outer function, or even on the top level?
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.
good point. Adding a comment for now.
internal/store/s3.go
Outdated
Key: aws.String(probeKey), | ||
}) | ||
if err != nil { | ||
// we were able to write this object, it must exists. |
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.
But there was still an error? so I'm a little confused by workflow here
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.
Update the comment. This workflow is just to verify that we can write, read, delete, list an object.
} | ||
} | ||
} | ||
|
||
// Store represents a destination to perform a backup/restore. | ||
type Store interface { |
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.
Can you rename store. Since that usually means a hard drive. ExternalStorage or something like that perhaps?
Introduce `internal/store/s3.go` with an S3-backed Store implementation: - Constructed via `S3FromEnv` using AWS-style environment variables. - Supports alternates (path-style, skip checksum, skip TLS) with a safe baseline first. - Probes connectivity by writing, reading, and deleting a `_blobcheck` object.
Introduce
internal/store/s3.go
with an S3-backed Store implementation:S3FromEnv
using AWS-style environment variables._blobcheck
object.