Skip to content

Conversation

jamiehannaford
Copy link
Contributor

Fixes #455

@@ -196,7 +196,7 @@ public function setTenant($tenant)
{
$identity = IdentityService::factory($this);

if (is_numeric($tenant)) {
if (is_numeric($tenant) || is_string($tenant)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you update the docblock for the method to match this change, please?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure - will do this and tests sometime tomorrow

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I should've been clearer. I meant the comment that goes "If an integer is passed in..."

@ycombinator
Copy link
Contributor

Could you please add unit tests for setTenant with string and numeric tenant IDs to https://github.com/jamiehannaford/php-opencloud/blob/import-tenant-fix/tests/OpenCloud/Tests/OpenStackTest.php?

@jamiehannaford
Copy link
Contributor Author

@ycombinator Looks like we already test string and numeric tenant IDs but the existing tests aren't really testing what we want it to test. If the argument does not match the conditional (which it was doing before), the value populates the Tenant object. If Base.php is given a scalar value, it treats it as a ID and does a GET on that resource to get more information - which is what was happening here.

Ideally, what we want to test is that strings and IDs do not execute this GET request. The only way we can efficiently do this is by mocking the exact form of communication the OpenCloud\OpenStack object has with its collaborators; i.e. make sure this client does not invoke a particular method with a particular string of arguments. We don't have this level of mocking right now, but it's definitely something to look into for v2.

@jamiehannaford
Copy link
Contributor Author

@ycombinator and this!

ycombinator added a commit that referenced this pull request Nov 19, 2014
Fixing import issue - tenant IDs can be strings
@ycombinator ycombinator merged commit 1c40e5c into rackspace:working Nov 19, 2014
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.

Caching credentials using Openstack
2 participants