search

dell-asm / dell-compellent

Apache License 2.0
3 stars 25 forks source link

ASM-9485 Deploy compellent with "Server Cluster" settings to VMware cluster #52

Closed sushilrai closed 7 years ago

sushilrai commented 7 years ago

Added support for cluster server object so that volume mapping can be done to the cluster server object instead of individual server object. Manifest file is updated with new workflow

sushilrai commented 7 years ago

@kaushalgala I have tried to address all code comments apart from adding the tests. This will take some time to add tests for all type and providers. I can do the same in separate PR

aimers1975 commented 7 years ago

@sushilrai Are you making further changes to this, QA is blocked until we get this Compellent issue resolved?

sushilrai commented 7 years ago

@aimers1975 @gavin-scott I am not making any more changes to this PR. Will add rspec tests in separate PR.

aimers1975 commented 7 years ago

@gavin-scott Are we still waiting on changes from @sushilrai on this one?

gavin-scott commented 7 years ago

@aimers1975 the PR is still marked as changes requested from @kaushalgala

@kaushalgala are you OK to merge?

I would prefer to address the potential security issue (command injection) I mentioned in this particular PR but if we've already agreed to do unit tests separately this can be merged as is and that can be added separately.

Deployments using server objects instead of cluster objects might succeed without this change (with the other two PRs that were already merged) but I have no way of checking.

gavin-scott commented 7 years ago

@sushilrai please look at adding unit tests and changing the bash commands to be command+list of args (no shell) in a follow-on PR.

kaushalgala commented 7 years ago

Thanks @sushilrai for addressing all comments.

Yes, it's ok for now to add unit tests in separate PR with current state of things.

Next time no exceptions please :)