Skip to content

Conversation

@FernandoOjeda
Copy link
Contributor

@FernandoOjeda FernandoOjeda commented Feb 20, 2020

Fix block and file storage detail #1229.
This works for block and file storage with id and name identifier.

Valid name:
slcli -v file volume-detail SL01SEV30_1

Valid id
slcli -v file volume-detail 11111

Bad name
slcli -v file volume-detail test
Error: Unable to find File Storage 'test'

@coveralls
Copy link

coveralls commented Feb 20, 2020

Coverage Status

Coverage increased (+0.01%) to 93.838% when pulling b07e0ec on FernandoOjeda:fo_block_file_storage_detail into 8c87513 on softlayer:master.

@allmightyspiff allmightyspiff added Bug and removed CLI labels Feb 20, 2020
Copy link
Member

@allmightyspiff allmightyspiff left a comment

Choose a reason for hiding this comment

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

  1. Move the id resolver to the managers
  2. make sure to pass in just the id for the list volumes method
  3. Make sure to pass in a username to the list volumes methods
  4. Only need to test for a few strings, not the whole output
  5. Test to make sure the appropriate API calls were called.

from SoftLayer import utils


def get_block_volume_id(volume_id, block_manager):
Copy link
Member

Choose a reason for hiding this comment

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

lets move this to the block and file managers as _get_ids_from_username(username)

Then add it as a resolver to the manager itself.

class BlockStorageManager(utils.IdentifierMixin, object):
    def __init__(self, client):
        self.configuration = {}
        self.client = client
        self.resolvers = [self._get_ids_from_username]

:param block_manager: Block Storage Manager.
:return: Returns the volume id.
"""
storage_list = block_manager.list_block_volumes()
Copy link
Member

Choose a reason for hiding this comment

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

list_block_volumes() takes in a username parameter to filter by that, which will let you only get the volume back that matches the username. Also pass in a mask of just id help reduce API call returned data.

"""Display details for a specified volume."""
block_manager = SoftLayer.BlockStorageManager(env.client)
block_volume = block_manager.get_block_volume_details(volume_id)
volume_id = get_block_volume_id(volume_id, block_manager)
Copy link
Member

Choose a reason for hiding this comment

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

since were moving get_block_volume_id to the block manager's resolvers feature, we can just remove this line and the rest should work.

result = self.run_command(['file', 'volume-detail', 'user'])

self.assert_no_fail(result)
self.assertEqual({
Copy link
Member

Choose a reason for hiding this comment

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

We don't really need to test the WHOLE output is equal, that will make this test break anytime we change the fixture.

Instead you can do something like this to test a few fields of the output

    self.assert_called_with('SoftLayer_Network_Storage', 'getObject')
    self.assertIn('username', result.output)
    self.assertIn('READHEAVY_TIER', result.output)

or whatever other strings you want to look for to make sure it called the correct data.

@allmightyspiff allmightyspiff linked an issue Feb 21, 2020 that may be closed by this pull request
allmightyspiff added a commit to allmightyspiff/softlayer-python that referenced this pull request Feb 26, 2020
allmightyspiff added a commit to allmightyspiff/softlayer-python that referenced this pull request Feb 26, 2020
@allmightyspiff
Copy link
Member

I've added your changes to #1235

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

slcli [block|file] volume-detail <bad name> throws exception

3 participants