-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
providers/aws: S3 bucket website support #1738
Conversation
justincampbell
commented
Apr 29, 2015
- Create/update/destroy
- index/error document
- Computed endpoint
- Multi-region support
- Docs
} | ||
if err := d.Set("website_endpoint", endpoint); err != nil { | ||
return 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.
This doesn't seem to set website_endpoint
to ""
if you disable website support on an existing bucket. How do I trigger other resources that depend on that attribute to be updated?
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'd actually just add an acceptance test that includes the disable case to test this. So it'd be a test with one step that has website support and the next step that removes it.
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 in bb63f5c
I think a |
e3dee1b
to
a0b3b5a
Compare
|
||
endpoint = fmt.Sprintf("%s.s3-website-%s.amazonaws.com", bucket, region) | ||
|
||
return |
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 is just a personal preference, but I prefer explicit returns when possible. I don't think that's a requirement, but I believe most of TF is consistent there.
Looks solid! 👍 |
460d8c2
to
9151e3c
Compare
Are we OK with this PR as is, with #1769 being a separate PR? |
It looks like the |
Also fix when index/error document is empty
@mitchellh done! |
Alright this looks ready to 🚢 🙆♀️ |
providers/aws: S3 bucket website support
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |