Allow user to set redirect options for the bucket #23

linusmarco · 2018-02-20T13:23:49Z

This PR implements the feature I described in #21.

Specifically, the user is now allowed to set all redirect/routing options supported by the CloudFormation API:
https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-s3-websiteconfiguration.html

Note that I had to slightly change the implementation of the recent error/index document options because CloudFormation does not allow the optional RedirectAllRequestsTo property to be specified with any other WebsiteConfiguration options. Therefore, we cannot set IndexDocument and ErrorDocument by default; instead those now get set only if RedirectAllRequestsTo is not specified. Existing behavior is unchanged (for these options and everything else).

Thanks!

…ndo-mc#21)

fernando-mc · 2018-02-27T05:51:34Z

@linusmarco It's taken me a while to get to this, sorry about that! I'll try to review this over the weekend.

linusmarco · 2018-02-27T14:47:58Z

No problem at all. That sounds good to me!

evanseeds · 2018-03-01T14:54:41Z

@linusmarco oh nice, I didn't realize this CloudFormation option was available. What's the use case for RedirectAllRequestsTo? I first thought it might be a natural solution for single-page applications, better than setting both indexDocument and errorDocument to the same page. Looking at the AWS documentation, however, it seems like RedirectAllRequestsTo is used to redirect to a different host, not to a specific document.

In the indexDoc/errorDoc PR I submitted, I originally had a spa option to enable a single-page application configuration, which we removed for clarity's sake. Would these options provide a better practice for those situations?

Sorry if this is a little off-topic from the PR itself!

linusmarco · 2018-03-01T16:55:14Z

@evanseeds -- I'm honestly not sure what a real-world use case would be for the RedirectAllRequestsTo option -- maybe if you moved a site to a different domain or something? I just included that option in this PR for completeness' sake.

Using RoutingRules is a more flexible alternative to the trick of setting the errorDocument to the same file as the indexDocument and can be used in conjunction with that trick. For instance, to achieve the same base result as indexDoc=errorDoc, you could do:

routingRules:
   - redirect:
       hostName: www.my-website.com
       httpRedirectCode: 300
       protocol: https
       replaceKeyPrefixWith: '#/'
     condition:
       httpErrorCodeReturnedEquals: 404

The big use case for RoutingRules (for me at least) is to direct certain requests to Lambda functions. That way in my front-end code I can send a request to 'www.my-website.com/api/users' and have the S3 bucket autmatically route that to the correct Lambda function via an API Gateway endpoint:

routingRules:
   ...
   - redirect:
       hostName: gatewayhash.execute-api.us-east-1.amazonaws.com
       httpRedirectCode: 300
       protocol: https
       replaceKeyPrefixWith: 'production/'
     condition:
       keyPrefixEquals: 'api/'

But there's a bunch of other useful stuff you can do with these options too!

fernando-mc

I few general suggestions on how we can improve the structure. Address those and I'll try to test this for merge Wednesday.

fernando-mc · 2018-03-03T17:47:26Z

README.md

    indexDocument: index.html # (Optional) The name of your index document inside your distributionFolder. Defaults to index.html
    errorDocument: error.html # (Optional) The name of your error document inside your distributionFolder. Defaults to error.html
+    redirectAllRequestsTo: # (Optional) If specified, all requests will redirect to specified endpoint
+      hostName: [hostName] # (Required) Name of the host where requests are redirected (e.g. www.google.com)  
+      protocol: [http|http] # (Optional) Protocol for redirect


Fix - http|http --> http|https?

good catch!

fernando-mc · 2018-03-03T17:48:27Z

README.md

+    routingRules: # (Optional) Redirect routing rules
+      - redirect: # (Required) Redirect options for this rule
+          hostName: [hostName] # (Optional) Name of the host where requests are redirected (e.g. www.google.com)  
+          httpRedirectCode: [CODE] # (Optional) HTTP status code for redirect


add example codes? Or is this all covered in the AWS docs. I'm fine with linking to that in multiple places this if it is.

I don't think they spell it out in the AWS docs, so I'll give some examples here

fernando-mc · 2018-03-03T17:48:46Z

README.md

+      - redirect: # (Required) Redirect options for this rule
+          hostName: [hostName] # (Optional) Name of the host where requests are redirected (e.g. www.google.com)  
+          httpRedirectCode: [CODE] # (Optional) HTTP status code for redirect
+          protocol: [http|http] # (Optional) Protocol for redirect


Same fix as above. one should be https?

fernando-mc · 2018-03-05T04:08:18Z

index.js

+
+      }
+
+      function confirm(expr, error) {


This is kinda awkward and uses the same name as the built-in browser function confirm().

What do you think about using something like this - https://github.com/arasatasaygin/is.js

Might still need a little custom work to implement it but seems like it might save us defining a bunch of custom checks. If we're going to define those they should be broken out of an already unwieldy index.js

Whoops, I had completely forgotten that confirm was a built-in...I never really use it. The library you linked looks like a great option for the kind of stuff that I was rolling-my-own on. I'm happy to switch to that. Thanks!

linusmarco · 2018-03-06T00:03:22Z

@fernando-mc - I addressed your comments. Thanks for the review!

fernando-mc · 2018-03-06T01:10:39Z

@linusmarco awesome! Last thing, can you please update to conform with the style guidelines PR you put in? I figured it would be better to get the style guidelines in sooner since they would basically make everything have a merge conflict.

linusmarco · 2018-03-08T02:25:57Z

@fernando-mc - updated coding and docs style to comply with recently merged PR. Should be ready to merge.

linusmarco · 2018-03-15T01:16:24Z

@fernando-mc - Updated to fix merge conflicts resulting from #42

Tested and should be ready to merge

linusmarco · 2018-03-15T01:32:05Z

Just updated this PR to merge to the new development branch rather than master

fernando-mc · 2018-03-15T05:32:51Z

@linusmarco, awesome work! I’ll take a look at everything tomorrow. Hoping to run a set of tests on all PRs that are ready for it then we can merge into development!

fernando-mc · 2018-04-22T20:27:05Z

@linusmarco I think I dropped the ball on this. Can we salvage the code changes from this and move forward on this again after I merge up v2?

linusmarco · 2018-04-22T22:22:59Z

@fernando-mc I actually worked these changes into the refactor (#43), so no need to do anythong with this PR. I'll close it.

linusmarco added 2 commits February 20, 2018 08:13

add ability to specify redirect rules for hosted bucket (closes ferna…

91afab9

…ndo-mc#21)

update README with documentation for redirect options

d8908aa

fernando-mc self-assigned this Feb 28, 2018

evanseeds mentioned this pull request Mar 1, 2018

Reworking documentation #30

Closed

fernando-mc reviewed Mar 5, 2018

View reviewed changes

linusmarco self-assigned this Mar 5, 2018

linusmarco added 2 commits March 5, 2018 18:58

fix README typos

3fda950

add is_js to replace awkward input checking

4f81637

linusmarco added 2 commits March 7, 2018 21:19

merge recent linting changes and update documentation style

adf7866

undo unwanted README reformatting

76b8a6d

linusmarco added 2 commits March 9, 2018 20:07

adding back in require that got lost in merge

810fe9c

merge changes from master

361ae86

linusmarco changed the base branch from master to development March 15, 2018 01:31

merge in recent changes from development branch

7e56a59

linusmarco mentioned this pull request Apr 7, 2018

Refactor #43

Merged

linusmarco closed this Apr 22, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow user to set redirect options for the bucket #23

Allow user to set redirect options for the bucket #23

linusmarco commented Feb 20, 2018

fernando-mc commented Feb 27, 2018

linusmarco commented Feb 27, 2018 •

edited

Loading

evanseeds commented Mar 1, 2018

linusmarco commented Mar 1, 2018 •

edited

Loading

fernando-mc left a comment

fernando-mc Mar 3, 2018

linusmarco Mar 5, 2018

fernando-mc Mar 3, 2018

linusmarco Mar 5, 2018

fernando-mc Mar 3, 2018

linusmarco Mar 5, 2018

fernando-mc Mar 5, 2018

linusmarco Mar 5, 2018

linusmarco commented Mar 6, 2018

fernando-mc commented Mar 6, 2018

linusmarco commented Mar 8, 2018

linusmarco commented Mar 15, 2018

linusmarco commented Mar 15, 2018

fernando-mc commented Mar 15, 2018

fernando-mc commented Apr 22, 2018

linusmarco commented Apr 22, 2018 •

edited

Loading

Allow user to set redirect options for the bucket #23

Allow user to set redirect options for the bucket #23

Conversation

linusmarco commented Feb 20, 2018

fernando-mc commented Feb 27, 2018

linusmarco commented Feb 27, 2018 • edited Loading

evanseeds commented Mar 1, 2018

linusmarco commented Mar 1, 2018 • edited Loading

fernando-mc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linusmarco commented Mar 6, 2018

fernando-mc commented Mar 6, 2018

linusmarco commented Mar 8, 2018

linusmarco commented Mar 15, 2018

linusmarco commented Mar 15, 2018

fernando-mc commented Mar 15, 2018

fernando-mc commented Apr 22, 2018

linusmarco commented Apr 22, 2018 • edited Loading

linusmarco commented Feb 27, 2018 •

edited

Loading

linusmarco commented Mar 1, 2018 •

edited

Loading

linusmarco commented Apr 22, 2018 •

edited

Loading