Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add website_domain for S3 buckets. #2210

Merged
merged 1 commit into from
Jul 22, 2015
Merged

Add website_domain for S3 buckets. #2210

merged 1 commit into from
Jul 22, 2015

Conversation

johnrengelman
Copy link
Contributor

Solution for #2208

@johnrengelman
Copy link
Contributor Author

Anyone comments on this?

@radeksimko
Copy link
Member

Thanks for this, I bumped into this issue yesterday too.

radeksimko
radeksimko reviewed Jun 16, 2015
View reviewed changes
builtin/providers/aws/resource_aws_s3_bucket.go
@@ -244,6 +251,9 @@ func resourceAwsS3BucketRead(d *schema.ResourceData, meta interface{}) error {
if err := d.Set("website_endpoint", endpoint); err != nil {
return err
}
if err := d.Set("website_domain", strings.TrimPrefix(endpoint, fmt.Sprintf("%s.", d.Id()))); err != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of trimming a string we build elsewhere, why not build the domain first and use that domain in website_endpoint?
Considering we have to call the s3conn.GetBucketLocation (i.e. we need the bucket name to get the domain), I would consider creating a new type, e.g.

type S3Website struct {
  Endpoint, Domain string
}

and then change websiteEndpoint(s3conn *s3.S3, d *schema.ResourceData) (string, error) to website(s3conn *s3.S3, d *schema.ResourceData) (S3Website, error).

That should be efficient enough since we only need to call the function once and get both domain+endpoint.

@catsby
Copy link
Contributor

catsby commented Jun 26, 2015

Hey @johnrengelman are you planning on incorporating Radek's feedback, or do you want to leave as is?

@catsby catsby added the waiting-response An issue/pull request is waiting for a response from the community label Jun 26, 2015
@johnrengelman
Copy link
Contributor Author

I'll incorporate the feedback...Just need to find some time to do it. So unless you are looking for stuff to do, I should have this updated in the next couple days.

@catsby
Copy link
Contributor

catsby commented Jun 26, 2015

excellent, thanks for following up!

@johnrengelman
Copy link
Contributor Author

@catsby @radeksimko - I finally got a around to incorporating those comments into this.

@radeksimko radeksimko removed the waiting-response An issue/pull request is waiting for a response from the community label Jul 16, 2015
catsby
catsby reviewed Jul 21, 2015
View reviewed changes
website/source/docs/providers/aws/r/s3_bucket.html.markdown
@@ -67,3 +67,4 @@ The following attributes are exported:
* `hosted_zone_id` - The [Route 53 Hosted Zone ID](http://docs.aws.amazon.com/general/latest/gr/rande.html#s3_website_region_endpoints) for this bucket's region.
* `region` - The AWS region this bucket resides in.
* `website_endpoint` - The website endpoint, if the bucket is configured with a website. If not, this will be an empty string.
* `website_domain` - The domain of the website endpoint, if teh bucket is configured with a website. If not, this will be an empty string. This is used to create Route 53 alias records.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if teh bucket

😄

@catsby
Copy link
Contributor

catsby commented Jul 21, 2015

Hey @johnrengelman sorry for dragging my feet here; just 2 nitpicks and things looks good otherwise

@catsby catsby added the waiting-response An issue/pull request is waiting for a response from the community label Jul 21, 2015
@johnrengelman
Copy link
Contributor Author

@catsby cleanup this up based on your comments and rebased.

catsby
catsby reviewed Jul 22, 2015
View reviewed changes
builtin/providers/aws/resource_aws_route53_record_test.go
bucket = "website.notexample.com"
acl = "public-read"
website {
index_document = index.html
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

index.html needs to be in quotes, or the test fails to parse the config

@catsby
Copy link
Contributor

catsby commented Jul 22, 2015

Hey @johnrengelman sorry to drag you on, but I tried running the test and beyond the parse issue, I got this result:

--- FAIL: TestAccAWSRoute53Record_s3_alias (106.20s)
    testing.go:136: Step 0 error: Error applying: 1 error(s) occurred:

        * 1 error(s) occurred:

        * 1 error(s) occurred:

        * InvalidChangeBatch: Tried to create an alias that targets s3-website-us-west-2.amazonaws.com., type A in zone ZVR5AG0O4FRPM, but the alias target name does not lie within the target zone
            status code: 400, request id: [d699b7f2-3085-11e5-be68-61e8c41f077e]

@johnrengelman
Copy link
Contributor Author

@catsby just pushed a fix for that. Looks like a copy paste error on my part.

@catsby
Copy link
Contributor

catsby commented Jul 22, 2015

Passing now, thanks!

catsby added a commit that referenced this pull request Jul 22, 2015
@catsby
Merge pull request #2210 from PeopleNet/s3-domain
8437006 
Add website_domain for S3 buckets.
@catsby catsby merged commit 8437006 into hashicorp:master Jul 22, 2015
@catsby
Copy link
Contributor

catsby commented Jul 22, 2015

Merged – sorry for all the run around, I appreciate the extra work 😄

@johnrengelman
Copy link
Contributor Author

No worries. That's how open source should work.

@code-later code-later mentioned this pull request Sep 27, 2015
Closed
@johnrengelman johnrengelman deleted the s3-domain branch September 28, 2015 19:19
@ghost
Copy link

ghost commented Apr 30, 2020

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.

@ghost ghost locked and limited conversation to collaborators Apr 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
enhancement provider/aws waiting-response An issue/pull request is waiting for a response from the community
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@johnrengelman @radeksimko @catsby