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

Proxy_url & connection object fixes #578

Closed
wants to merge 5 commits into
base: trunk
from

Conversation

Projects
None yet
2 participants
@MrBasset
Contributor

MrBasset commented Sep 14, 2015

I have a use case that requires explicitly setting the proxy_url for accessing a vCloud API (as opposed to the default environment proxy). I noticed that the proxy_url was not being passed to the base Connection class by the base NodeDriver. I fixed this by adding the proxy_url into the constructor call where the connection class is created.

Also, within the vCouldNodeDriver a new connection class we being created with
no custom variables rather than re-using the existing connection class. Any custom values, such as the proxy_url, were not being passed to this new instance. By swapping to the self.connection class these variables are set.

I could not find any unit tests that would demonstrate this issue (I would assume that they would be integration tests anyway). So apologies that this fix does not have supporting tests.

MrBasset added some commits Sep 11, 2015

Proxy_url & connection object fixes
The proxy_url was not being passed to the base Connection class by the
base NodeDriver.

Within the vCouldNodeDriver a new connection class we being created with
no custom variables rather than re-using the existing connection class.

Signed-off-by: MrBasset <daniel.pool@gmail.com>
Omission in changing references
Missed changing a reference to a variable that has been removed.
@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Sep 20, 2015

Member

Thanks. The changes look good to me.

Looks like the build is failing - https://travis-ci.org/apache/libcloud/builds/80160975. Can you please fix this?

Member

Kami commented Sep 20, 2015

Thanks. The changes look good to me.

Looks like the build is failing - https://travis-ci.org/apache/libcloud/builds/80160975. Can you please fix this?

@MrBasset

This comment has been minimized.

Show comment
Hide comment
@MrBasset

MrBasset Sep 21, 2015

Contributor

Sorry, will look at what I've done wrong.

Contributor

MrBasset commented Sep 21, 2015

Sorry, will look at what I've done wrong.

MrBasset added some commits Sep 22, 2015

Changes to remove __init__ masking access to base class proxy_url
Unit tests were failing as specific Connection classes were masking
access to the proxy_url property of the base Connection class.
More lint errors
Damn it ... I need to install lint ...
@MrBasset

This comment has been minimized.

Show comment
Hide comment
@MrBasset

MrBasset Sep 22, 2015

Contributor

Kami,

I've managed to get a green light on the build - sorry it took so long. It seems that by adding that parameter into the base connection class, has highlighted that a number of the vendor specific drivers also didn't pass down the proxy_url parameter. I'm not sure if explicitly defining the proxy_url is the best approach, or whether it would be better to use **kwargs etc? I'd assumed the former, hopefully this isn't a breaking change.

If this change is too wide reaching I'm happy to take advice on how to better implement it.

Contributor

MrBasset commented Sep 22, 2015

Kami,

I've managed to get a green light on the build - sorry it took so long. It seems that by adding that parameter into the base connection class, has highlighted that a number of the vendor specific drivers also didn't pass down the proxy_url parameter. I'm not sure if explicitly defining the proxy_url is the best approach, or whether it would be better to use **kwargs etc? I'd assumed the former, hopefully this isn't a breaking change.

If this change is too wide reaching I'm happy to take advice on how to better implement it.

@asfgit asfgit closed this in 3fa2e1b Sep 25, 2015

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Sep 25, 2015

Member

Merged, thanks.

Member

Kami commented Sep 25, 2015

Merged, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment