-
Notifications
You must be signed in to change notification settings - Fork 6
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
Changes to verify if given entity exists in Nuage VM table #3
Conversation
var uuidRows []map[string]interface{} | ||
var err error | ||
if uuidRows, err = vrsConnection.vmTable.ReadRows(vrsConnection.ovsdbClient, readRowArgs); err != nil { | ||
return false, fmt.Errorf("Unable to verify if entity exists in OVSDB %v", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change error string to "OVSDB read error"
uuids = append(uuids, uuid[ovsdb.NuageVMTableColumnVMUUID].(string)) | ||
} | ||
|
||
if uuid == uuids[0] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need to check the count of uuids before this check?
return true, nil | ||
} | ||
|
||
return true, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this return false?
@@ -221,3 +221,28 @@ func (vrsConnection *VRSConnection) GetAllEntities() ([]string, error) { | |||
|
|||
return uuids, nil | |||
} | |||
|
|||
// CheckEntity verifies if a specified entity exists in VRS | |||
func (vrsConnection *VRSConnection) CheckEntity(uuid string) (bool, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change function name to EntityExists
} | ||
|
||
var uuids []string | ||
for _, uuid := range uuidRows { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand go's scoping rules will hide the uuid declared in the for loop iteration from that passed to the function as argument. But for readability purposes, I would encourage you to rename the variable in the loop iteration to something else.
if entityExists { | ||
t.Logf("VM %s with UUID %s exists in OVSDB", vmInfo["name"], vmInfo["vmuuid"]) | ||
} else { | ||
t.Fatalf("VM %s with UUID %s doesnot exist in OVSDB", vmInfo["name"], vmInfo["vmuuid"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does not
vs. doesnot
uuids = append(uuids, uuid[ovsdb.NuageVMTableColumnVMUUID].(string)) | ||
} | ||
|
||
if len(uuids) > 0 && uuid == uuids[0] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we expect uuids to be unique, is there a need for allowing len(uuids) > 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a check for length to handle the scenario wherein no such UUID exists in OVSDB (for new entity) then "uuids" will be blank and program will panic trying to access uuids[0]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant you can always make sure that the len(uuids) == 1
} | ||
|
||
if entityExists { | ||
t.Logf("VM %s with UUID %s exists in OVSDB", vmInfo["name"], vmInfo["vmuuid"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Entity %s with ID %s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this testcase; we are specifically creating a VM entity; so I think we are being more specific by stating "VM" and UUID instead of entity/id in the logs
if entityExists { | ||
t.Logf("VM %s with UUID %s exists in OVSDB", vmInfo["name"], vmInfo["vmuuid"]) | ||
} else { | ||
t.Fatalf("VM %s with UUID %s does not exist in OVSDB", vmInfo["name"], vmInfo["vmuuid"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above line 321
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
Changes to verify if given entity exists in Nuage VM table