Added support for AWS CloudFront (or any other CDN)

[eriktaubeneck]

This allows you to change the base domain to anything and won't include the S3 bucket name before the url, as it does when changing S3_BUCKET_DOMAIN. I updated the docs (so you'll want to update the readthedocs.org site) and added a test for the feature.

[eriktaubeneck]

@e-dard - a mention so that you see this!

[eriktaubeneck]

@e-dard Any chance you can look at this?

[e-dard]

How about we just check if S3_BUCKET_NAME is not None and only then prepend S3_BUCKET_NAME and . to S3_BUCKET_DOMAIN?

[e-dard]

Hey, thanks for the PR!

I think we could avoid adding another configuration parameter by making some changes to how we build up the full bucket_path. If you check S3_BUCKET_NAME is not empty or None you could then build bucket_path with or without the dot.

That way, the use-case for another CDN would simply be:

S3_BUCKET_NAME = None
S3_BUCKET_DOMAIN = "foo.cloudfront.net"

It would probably be a good idea to add a section underneath Static Asset URLs, say called Using Alternative CDNs, that briefly outlines how to configure Flask S3 to achieve the functionality your PR provides.

One other thing. After taking a look at @hannseman's PR, I think there are going to be some changes that I will merge first, thus requiring you to rebase. It's hard to avoid each other when there are only a couple of files to hack on!

[eriktaubeneck]

The only issue with that is that you would break the ability to upload your static files to S3, which would be the point of using Flask-S3. You could change the url_for path for a CDN in a much simpler manner if that was your only goal.

With respect to @hannseman's PR, been watching that and perfect fine rebasing that in. I suggest we only bump the version number once for both of these changes, and push them together to PyPi, unless you have some reason not to.

[e-dard]

I probably shouldn't try and review PRs at 1am and expect to provide intelligent comments.

I didn't really understand the use-case of CloudFront or another CDN, but having just gone through some CF documentation I think I do now.

Basically, with CF (and I assume other 3rd party CDNs) you can upload your assets to S3 and let the CDN serve them out, rather than serving them out from S3 directly. If that's the case then of course we want to maintain the uploading of assets to S3 in the current way, and only change the URL that Flask-S3 inserts into templates, to match the CDN domain.

If that's the case then your solution seems fine. One final thing — could you revert the version bump as there is some other house cleaning I want to do on the project after I merge current PRs. I'll then bump and re-release to Pypi etc.

[eriktaubeneck]

No worries! After reading your comment, I thought of another possible solution, but it would break the existing API. The existing S3_BUCKET_DOMAIN could overwrite the domain completely, and then if you wanted to change the domain, but include the bucket name, you would do

app.config['S3_BUCKET_NAME'] = 'foo'
app.config['S3_BUCKET_DOMAIN'] = '%s.%s.' % (app.config['S3_BUCKET_NAME'], 'bar.domain.com')

and the CDN usage would be

app.config['S3_BUCKET_NAME'] = 'foo'
app.config['S3_BUCKET_DOMAIN'] = bar.cloudfront.net'

Let me know if you like this solution better, and then I will rebase, revert the version bump, and resubmit.

[eriktaubeneck]

So I think this should be good now (although I think my rebase is making @hannseman's commits show up here as well...). This should be good, unless you want to go the route that I suggested a few hours ago.

[e-dard]

I think I prefer your original solution because it's more explicit with the variables (rather than overwriting S3_BUCKET_DOMAIN, which, on reflection, would change the semantics.

Did you rebase from e-dard/flask-s3:master ? I get this issue with extra commits at work sometimes but I can never remember what I did wrong to get it like that 😆

[eriktaubeneck]

Agreed on being explicit.

I pulled from e-dard/flask-s3:master into an upstream branch, and then rebased against it. It tried to merge the first time around, and it looks like it gave a few commits a different hash. Really weird. Let me try to get rid of that.

[eriktaubeneck]

Yay it's fixed! 👍

[e-dard]

Cool. Thanks for this!

[eriktaubeneck]

Thanks for writing it to begin with! Cheers