-
Notifications
You must be signed in to change notification settings - Fork 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
Basic S3 support. #14
Conversation
setting access keys is optional, if they are unset, the implicit credentials from your environment will be used.
|
||
s3: | ||
enabled: false | ||
bucket: "<S3-BUCKET-NAME>" |
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'm not the worlds biggest fan of <THESE-DEFAULT-PLACEHOLDERS>
but I don't have a good alternative. I figure that this can be improved in a later pass if it ever is an issue.
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.
Agreed.
@@ -58,9 +58,6 @@ deck: | |||
# hosts: | |||
# - domain.com | |||
|
|||
# Bucket to use when storing config data in S3 compatible storage | |||
storageBucket: spinnaker |
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 don't really see the reason to have a shared root storageBucket
key and I think having it separate is less clear, so I denormalized it back into the individual config stanzas, but feel free to disagree with that, as I may be missing some context.
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.
This makes sense! Thanks for making the change. There was a great reason to have it split out other than the fact that there was only one provider originally and then I co-opted the existing value when I added GCS support.
@@ -58,9 +58,6 @@ deck: | |||
# hosts: | |||
# - domain.com | |||
|
|||
# Bucket to use when storing config data in S3 compatible storage | |||
storageBucket: spinnaker |
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.
This makes sense! Thanks for making the change. There was a great reason to have it split out other than the fact that there was only one provider originally and then I co-opted the existing value when I added GCS support.
|
||
s3: | ||
enabled: false | ||
bucket: "<S3-BUCKET-NAME>" |
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.
Agreed.
setting access keys is optional. if they are unset, the implicit credentials from your environment will be used.
Since this came from a downstream repo there's a small chance that my merge was bad, but I double checked and it looked fine.