Skip to content

Conversation

minishrink
Copy link
Contributor

Previously, PIF.set_disallow_unplug raised clustering_enabled_on_network even when trying to set to the same value, if cluster objects were found, causing tests to fail unnecessarily. This PR makes PIF.set_disallow_unplug idempotent and updates the relevant tests to check this new behaviour.

Akanksha Mathur added 2 commits March 12, 2018 09:50
Signed-off-by: Akanksha Mathur <akanksha.mathur@citrix.com>
Signed-off-by: Akanksha Mathur <akanksha.mathur@citrix.com>
Copy link
Contributor

@edwintorok edwintorok left a comment

Choose a reason for hiding this comment

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

LGTM, minor comment about a space.

let set_disallow_unplug ~__context ~self ~value =
let network = Db.PIF.get_network ~__context ~self in
if pif_has_clustering_enabled ~__context self network
if ((Db.PIF.get_disallow_unplug ~__context ~self) <> value)&& pif_has_clustering_enabled ~__context self network
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 add a space before the &&?

if pif_has_clustering_enabled ~__context self network
if ((Db.PIF.get_disallow_unplug ~__context ~self) <> value)&& pif_has_clustering_enabled ~__context self network
then raise Api_errors.(Server_error(clustering_enabled_on_network, [Ref.string_of network]))
else Db.PIF.set_disallow_unplug ~__context ~self ~value
Copy link
Contributor

Choose a reason for hiding this comment

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

You could skip this if you found it to be equal, but its fine either way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's a better way to do it.

Signed-off-by: Akanksha Mathur <akanksha.mathur@citrix.com>
@coveralls
Copy link

coveralls commented Mar 12, 2018

Coverage Status

Coverage increased (+0.003%) to 18.663% when pulling b59685c on minishrink:feature/REQ477/CA-285605 into db790e6 on xapi-project:feature/REQ477/master.

@edwintorok edwintorok merged commit f4c3a3a into xapi-project:feature/REQ477/master Mar 12, 2018
@minishrink minishrink deleted the feature/REQ477/CA-285605 branch April 19, 2018 14:46
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.

3 participants