-
Notifications
You must be signed in to change notification settings - Fork 4
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
Storage advisor #3
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.
Thanks; I think it's generally solid, but I would appreciate a few tweaks - in particular around logging.
|
||
FROM ubuntu AS bin | ||
|
||
COPY eu-central-1-bundle.crt /usr/local/share/ca-certificates/ |
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.
I believe we don't need this one - it's only necessary if we're talking to AWS RDS.
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.
Removed
|
||
ctx := context.TODO() | ||
cfg, err := config.LoadDefaultConfig(ctx) | ||
cfg.Region = "eu-central-1" |
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.
We probably should not hardcode region.
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.
Moved to flags but I don't see a reason to host it anywhere outside of eu-1
cfg, err := config.LoadDefaultConfig(ctx) | ||
cfg.Region = "eu-central-1" | ||
if err != nil { | ||
log.Println("Error:", err) |
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.
Since this is server-side code, I'd recommend we use zerolog for all logging. It produces json structured output, which is way easier to handle. Also, it has a fatal level, which we can use instead of explicit return or os.exit.
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.
Done
select { | ||
case sig := <-sigchan: | ||
log.Printf("Caught signal %v: terminating\n", sig) | ||
run = false |
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.
That's more of stylistic comment - but maybe "while true" look with "break" will be a bit cleaner.
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.
Done
} | ||
defer reader.Close() | ||
|
||
err = uploadToS3(cfg, ctx, reader, *bucketName, objectKey) |
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.
I wonder if we can use S3 CopyObject functionality here? I am not sure if that works with pre-signed URLs, but it if does, it will be more efficient than downloading and uploading.
@@ -0,0 +1,124 @@ | |||
-----BEGIN CERTIFICATE----- |
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.
And then we don't need this file.
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.
Removed
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.
Looks good enough to me; let's merge.
No description provided.