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

when a request is made to a CDN endpoint there is no internalURL to use,... #231

Closed

Conversation

@jobelenus
Copy link
Contributor

jobelenus commented Jan 29, 2014

... we have to fall back to the publicURL. This was an oversight on my part with my first pull request --#229

And this is the fix

…se, we have to fall back to the publicURL
@@ -105,6 +105,8 @@ class CloudFilesConnection(OpenStackBaseConnection):

auth_url = AUTH_URL
_auth_version = '2.0'
INTERNAL_URL = 'internalURL'

This comment has been minimized.

@Kami

Kami Jan 30, 2014 Member

Those attributes need better name.

On top of that, if they are upper case (constants) they should probably be stored on module level and not on class level.

This comment has been minimized.

@jobelenus

jobelenus Jan 30, 2014 Author Contributor

I was thinking about making them module level, but they only apply to the connection class.

self.endpoint_url = 'internalURL' if use_internal_url else 'publicURL'
self.use_internal_url = use_internal_url

def _get_endpoint_url(self):

This comment has been minimized.

@Kami

Kami Jan 30, 2014 Member

This method name and the variable name bellow is deceives. This method does not return an endpoint URL but a key which is used to look up the endpoint URL.

As such, the method and the variable need better names.

@asfgit asfgit closed this in 34c3df4 Jan 30, 2014
@Kami
Copy link
Member

Kami commented Jan 30, 2014

Thanks for making those changes.

There were some lint issues which I have fixed and merged patch into trunk. Next time please run tox -e lint and make sure there are no lint issues.

@jobelenus
Copy link
Contributor Author

jobelenus commented Jan 30, 2014

Ah thanks for that local command, I was always waiting for Travis to fail me :(

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

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.