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 support for unity host attach/detach unity lun #2

Merged
merged 11 commits into from
Jun 6, 2016

Conversation

crook
Copy link
Contributor

@crook crook commented May 25, 2016

Add support for unity host attach/detach unity lun

@jealous
Copy link
Contributor

jealous commented May 26, 2016

Hi @crook , the coverage dropped a little. It would be appreciated if you could remove the unused code or cover them with unit test. The detail data can be retrieved here:
https://coveralls.io/jobs/14804809
The main gap lies in the resource module.

return False

has = False
for host_lun in self.host_luns:
Copy link
Contributor

@jealous jealous May 26, 2016

Choose a reason for hiding this comment

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

These lines are duplicated.
I would suggest to create a property like this:

@properties
@instance_cache
def lun_ids(self):
    ret = []
    if self.host_luns:
        for host_lun in self.host_luns:
            if host_lun.lun:
                ret.append(host_lun.lun.id)
    return ret

def has_hlu(self, lun):
    return lun.id in self.lun_ids

The @instance_cache here works as an instance level memoize.

The code suggested here is a demonstration of the thoughts. Please try to converge the duplicated for loops.

@jealous
Copy link
Contributor

jealous commented May 26, 2016

Hi @crook ,
Nice work 👍. Very concise unit tests. Just some comments to be addressed.
And there is no need to open a new PR when you are done. I think just push your change will update this PR.

@jealous jealous self-assigned this May 26, 2016
@jealous jealous added the Unity label May 26, 2016
Ray Chen added 4 commits May 31, 2016 11:47
@coveralls
Copy link

coveralls commented Jun 3, 2016

Coverage Status

Coverage decreased (-0.06%) to 92.06% when pulling b61284a on crook:master into 5d3ab6f on emc-openstack:develop.

@coveralls
Copy link

coveralls commented Jun 6, 2016

Coverage Status

Coverage increased (+0.1%) to 92.243% when pulling 9c69181 on crook:master into 5d3ab6f on emc-openstack:develop.

@jealous jealous merged commit f0add3d into emc-openstack:develop Jun 6, 2016
@ghost ghost mentioned this pull request May 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants