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.

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

Add new tests to provide bug if blueprint with subdomain #20

Closed
wants to merge 1 commit into from

Conversation

klinkin
Copy link

@klinkin klinkin commented Feb 8, 2014

Problem with url_for function if set subdomain for blueprint.
Check new tests.

I don't now how to fix this problem(((

@klinkin
Copy link
Author

klinkin commented Feb 8, 2014

@e-dard and @eriktaubeneck could you look at it?

@eriktaubeneck
Copy link
Collaborator

Thanks for bringing this to our attention @klinkin. When I run these tests, I get

======================================================================
FAIL: Tests that correct url formed for static asset in blueprint with subdomain.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/erik/Documents/workspace/flask-s3/tests/test_flask_static.py", line 136, in test_url_for_blueprint_with_subdomain
    self.assertEquals(self.client_get(ufs).data, exp)
AssertionError: 'https://billing.foo.s3.amazonaws.com/billing-static/bah.js' != 'https://foo.s3.amazonaws.com/billing/billing-static/bah.js'

======================================================================
FAIL: test_url_for_cdn_domain_for_blueprint_with_subdomain (test_flask_static.UrlTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/erik/Documents/workspace/flask-s3/tests/test_flask_static.py", line 143, in test_url_for_cdn_domain_for_blueprint_with_subdomain
    self.assertEquals(self.client_get(ufs).data, exp)
AssertionError: 'https://billing.foo.cloudfront.net/billing-static/bah.js' != 'https://foo.cloudfront.net/billing/billing-static/bah.js'

----------------------------------------------------------------------

So for something that, without S3, would have a url of https://billing.barbaz.com/billing-static/bah.js. With S3 and a bucket name of foo, it becomes https://billing.foo.s3.amazonaws.com/billing-static/bah.js. This doesn't inherently seem wrong to me, but I'm not sure if there is an established pattern for this. @klinkin can you explain how you came up with https://foo.s3.amazonaws.com/billing/billing-static/bah.js as the expected value?

@klinkin
Copy link
Author

klinkin commented Feb 10, 2014

@eriktaubeneck
I think that it would be perfect to construct path to S3 like this template:

<shceme>:<bucket_name>.<amazon_url>/<subdomain>/<static_url>/<file_path>

Because subdomain billing.foo.s3.amazonaws.com not available.

@eriktaubeneck
Copy link
Collaborator

Ok cool. I don't disagree that it's a sufficient solution, was just curious if you had based it on an established pattern. Having the inability to guarantee that <subdomain>.<bucket_name>.<amazon_url> is available certainly rules that out, so your solution certainly seems quite reasonable. I'm going to do a little research to see if more established frameworks like Django or Rails have an established pattern for this.

@e-dard
Copy link
Owner

e-dard commented Feb 10, 2014

I'm a little confused; is this bug caused by #18 ?

@klinkin
Copy link
Author

klinkin commented Feb 10, 2014

No, it's an independent error.
Right now there is no possibility to use flask_s3 for blueprint with subdomain.
And i could not find solutions how fix this(((

@e-dard
Copy link
Owner

e-dard commented May 2, 2014

Doesn't pass CI and no activity on this issue for a while.

@e-dard e-dard closed this May 2, 2014
@eriktaubeneck
Copy link
Collaborator

@klinkin Can you not register a different bucket for each subdomain? That would seem to be the standard was to deal with it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants