LGTM with fixing the one place where you have the colon wrong. On Thu, Apr 23, 2015 at 1:15 PM, 'Helga Velroyen' via ganeti-devel < [email protected]> wrote:
> Interdiff for the docs: > > diff --git a/test/py/testutils_ssh.py b/test/py/testutils_ssh.py > index cc55a02..a0a0bae 100644 > --- a/test/py/testutils_ssh.py > +++ b/test/py/testutils_ssh.py > @@ -258,8 +258,7 @@ class FakeSshFileManager(object): > > @type key: string > @param key: key exptected to be present in all node's authorized_keys > file > - @rtype: boolean > - @return: True if key is present in all node's authorized_keys file > + @raise: Exception if a node does not have the authorized key. > Nit: you forgot to change this. > > """ > for name in self._all_node_data.keys(): > @@ -272,8 +271,7 @@ class FakeSshFileManager(object): > > @type key: string > @param key: key exptected to be present in all node's authorized_keys > file > - @rtype: boolean > - @return: True if key is not present in all node's authorized_keys file > + @raise Exception: if a node *does* have the authorized key. > > """ > for name in self._all_node_data.keys(): > @@ -287,8 +285,7 @@ class FakeSshFileManager(object): > > @type uuid: string > @param uuid: UUID of the node whose key is looked for > - @type key: string > - @param key: SSH key to be looked for > + @raise Exception: if a node *does* have the public key. > > """ > for name in self._all_node_data.keys(): > @@ -308,6 +305,8 @@ class FakeSshFileManager(object): > in the public key file of all potential master > candidates > @type query_node_name: string > + @raise Exception: when a potential master candidate does not have > + the public key or a normal node *does* have a > public key. > > """ > query_node_uuid, query_node_key, _, _, _ = \ > > > On Fri, 17 Apr 2015 at 16:37 Helga Velroyen <[email protected]> wrote: > >> So far, the (fake) SSH file manager had a couple of check >> functions (which return "True" or "False") and a couple of >> functions which throw an exception instead of returning >> "False" (as in those cases more information for debugging >> is neeede). However, it was not obvious from the function >> name which behavior to expect. This patch renames all >> functions which throw exceptions to "Assert*" and removes >> superfluous 'self.assertTrue(*)" calls around them. >> >> Signed-off-by: Helga Velroyen <[email protected]> >> --- >> test/py/ganeti.backend_unittest.py | 63 >> ++++++++++++++++---------------------- >> test/py/testutils_ssh.py | 11 +++---- >> 2 files changed, 30 insertions(+), 44 deletions(-) >> >> diff --git a/test/py/ganeti.backend_unittest.py b/test/py/ >> ganeti.backend_unittest.py >> index 804cd63..73db0a9 100755 >> --- a/test/py/ganeti.backend_unittest.py >> +++ b/test/py/ganeti.backend_unittest.py >> @@ -1119,10 +1119,9 @@ class >> TestAddRemoveGenerateNodeSshKey(testutils.GanetiTestCase): >> noded_cert_file=self.noded_cert_file, >> run_cmd_fn=self._run_cmd_mock) >> >> - self._ssh_file_manager.PotentialMasterCandidatesOnlyHavePublicKey( >> + >> self._ssh_file_manager.AssertPotentialMasterCandidatesOnlyHavePublicKey( >> new_node_name) >> - self.assertTrue(self._ssh_file_manager.AllNodesHaveAuthorizedKey( >> - new_node_key)) >> + self._ssh_file_manager.AssertAllNodesHaveAuthorizedKey(new_node_key) >> >> def testAddPotentialMasterCandidate(self): >> new_node_name = "new_node_name" >> @@ -1148,10 +1147,9 @@ class >> TestAddRemoveGenerateNodeSshKey(testutils.GanetiTestCase): >> noded_cert_file=self.noded_cert_file, >> run_cmd_fn=self._run_cmd_mock) >> >> - self._ssh_file_manager.PotentialMasterCandidatesOnlyHavePublicKey( >> + >> self._ssh_file_manager.AssertPotentialMasterCandidatesOnlyHavePublicKey( >> new_node_name) >> - self.assertTrue(self._ssh_file_manager.NoNodeHasAuthorizedKey( >> - new_node_key)) >> + self._ssh_file_manager.AssertNoNodeHasAuthorizedKey(new_node_key) >> >> def testAddNormalNode(self): >> new_node_name = "new_node_name" >> @@ -1177,10 +1175,8 @@ class >> TestAddRemoveGenerateNodeSshKey(testutils.GanetiTestCase): >> noded_cert_file=self.noded_cert_file, >> run_cmd_fn=self._run_cmd_mock) >> >> - self.assertTrue(self._ssh_file_manager.NoNodeHasPublicKey( >> - new_node_uuid, new_node_key)) >> - self.assertTrue(self._ssh_file_manager.NoNodeHasAuthorizedKey( >> - new_node_key)) >> + self._ssh_file_manager.AssertNoNodeHasPublicKey(new_node_uuid, >> new_node_key) >> + self._ssh_file_manager.AssertNoNodeHasAuthorizedKey(new_node_key) >> >> def testPromoteToMasterCandidate(self): >> # Get one of the potential master candidates >> @@ -1201,10 +1197,9 @@ class >> TestAddRemoveGenerateNodeSshKey(testutils.GanetiTestCase): >> noded_cert_file=self.noded_cert_file, >> run_cmd_fn=self._run_cmd_mock) >> >> - self._ssh_file_manager.PotentialMasterCandidatesOnlyHavePublicKey( >> + >> self._ssh_file_manager.AssertPotentialMasterCandidatesOnlyHavePublicKey( >> node_name) >> - self.assertTrue(self._ssh_file_manager.AllNodesHaveAuthorizedKey( >> - node_key)) >> + self._ssh_file_manager.AssertAllNodesHaveAuthorizedKey(node_key) >> >> def testRemoveMasterCandidate(self): >> (node_name, node_uuid, node_key, is_potential_master_candidate, >> @@ -1224,9 +1219,8 @@ class >> TestAddRemoveGenerateNodeSshKey(testutils.GanetiTestCase): >> noded_cert_file=self.noded_cert_file, >> run_cmd_fn=self._run_cmd_mock) >> >> - self.assertTrue(self._ssh_file_manager.NoNodeHasPublicKey( >> - node_uuid, node_key)) >> - >> self.assertTrue(self._ssh_file_manager.NoNodeHasAuthorizedKey(node_key)) >> + self._ssh_file_manager.AssertNoNodeHasPublicKey(node_uuid, node_key) >> + self._ssh_file_manager.AssertNoNodeHasAuthorizedKey(node_key) >> self.assertEqual(0, >> len(self._ssh_file_manager.GetPublicKeysOfNode(node_name))) >> self.assertEqual(0, >> @@ -1250,9 +1244,8 @@ class >> TestAddRemoveGenerateNodeSshKey(testutils.GanetiTestCase): >> noded_cert_file=self.noded_cert_file, >> run_cmd_fn=self._run_cmd_mock) >> >> - self.assertTrue(self._ssh_file_manager.NoNodeHasPublicKey( >> - node_uuid, node_key)) >> - >> self.assertTrue(self._ssh_file_manager.NoNodeHasAuthorizedKey(node_key)) >> + self._ssh_file_manager.AssertNoNodeHasPublicKey(node_uuid, node_key) >> + self._ssh_file_manager.AssertNoNodeHasAuthorizedKey(node_key) >> self.assertEqual(0, >> len(self._ssh_file_manager.GetPublicKeysOfNode(node_name))) >> self.assertEqual(0, >> @@ -1276,9 +1269,8 @@ class >> TestAddRemoveGenerateNodeSshKey(testutils.GanetiTestCase): >> noded_cert_file=self.noded_cert_file, >> run_cmd_fn=self._run_cmd_mock) >> >> - self.assertTrue(self._ssh_file_manager.NoNodeHasPublicKey( >> - node_uuid, node_key)) >> - >> self.assertTrue(self._ssh_file_manager.NoNodeHasAuthorizedKey(node_key)) >> + self._ssh_file_manager.AssertNoNodeHasPublicKey(node_uuid, node_key) >> + self._ssh_file_manager.AssertNoNodeHasAuthorizedKey(node_key) >> self.assertEqual(0, >> len(self._ssh_file_manager.GetPublicKeysOfNode(node_name))) >> self.assertEqual(0, >> @@ -1305,8 +1297,9 @@ class >> TestAddRemoveGenerateNodeSshKey(testutils.GanetiTestCase): >> noded_cert_file=self.noded_cert_file, >> run_cmd_fn=self._run_cmd_mock) >> >> - >> self._ssh_file_manager.PotentialMasterCandidatesOnlyHavePublicKey(node_name) >> - >> self.assertTrue(self._ssh_file_manager.NoNodeHasAuthorizedKey(node_key)) >> + >> self._ssh_file_manager.AssertPotentialMasterCandidatesOnlyHavePublicKey( >> + node_name) >> + self._ssh_file_manager.AssertNoNodeHasAuthorizedKey(node_key) >> >> def testDemotePotentialMasterCandidateToNormalNode(self): >> (node_name, node_uuid, node_key, is_potential_master_candidate, >> @@ -1329,9 +1322,8 @@ class >> TestAddRemoveGenerateNodeSshKey(testutils.GanetiTestCase): >> noded_cert_file=self.noded_cert_file, >> run_cmd_fn=self._run_cmd_mock) >> >> - self.assertTrue(self._ssh_file_manager.NoNodeHasPublicKey( >> - node_uuid, node_key)) >> - >> self.assertTrue(self._ssh_file_manager.NoNodeHasAuthorizedKey(node_key)) >> + self._ssh_file_manager.AssertNoNodeHasPublicKey(node_uuid, node_key) >> + self._ssh_file_manager.AssertNoNodeHasAuthorizedKey(node_key) >> >> def _GetReducedOnlineNodeList(self): >> """'Randomly' mark some nodes as offline.""" >> @@ -1424,10 +1416,10 @@ class >> TestAddRemoveGenerateNodeSshKey(testutils.GanetiTestCase): >> noded_cert_file=self.noded_cert_file, >> run_cmd_fn=self._run_cmd_mock) >> >> - self._ssh_file_manager.PotentialMasterCandidatesOnlyHavePublicKey( >> + >> self._ssh_file_manager.AssertPotentialMasterCandidatesOnlyHavePublicKey( >> new_node_name) >> - self.assertTrue(self._ssh_file_manager.AllNodesHaveAuthorizedKey( >> - new_node_key)) >> + self._ssh_file_manager.AssertAllNodesHaveAuthorizedKey( >> + new_node_key) >> >> def testAddKeyFailedOnNewNodeWithRetries(self): >> """Tests clean up if updating a new node's SSH setup fails. >> @@ -1465,8 +1457,7 @@ class >> TestAddRemoveGenerateNodeSshKey(testutils.GanetiTestCase): >> self.assertFalse(self._ssh_file_manager.NodeHasAuthorizedKey( >> node, new_node_key)) >> >> - self.assertTrue(self._ssh_file_manager.NoNodeHasPublicKey( >> - new_node_uuid, new_node_key)) >> + self._ssh_file_manager.AssertNoNodeHasPublicKey(new_node_uuid, >> new_node_key) >> >> def testAddKeySuccessfullyOnOldNodeWithRetries(self): >> """Tests adding a new key even if updating nodes takes retries. >> @@ -1499,8 +1490,7 @@ class >> TestAddRemoveGenerateNodeSshKey(testutils.GanetiTestCase): >> noded_cert_file=self.noded_cert_file, >> run_cmd_fn=self._run_cmd_mock) >> >> - self.assertTrue(self._ssh_file_manager.AllNodesHaveAuthorizedKey( >> - new_node_key)) >> + self._ssh_file_manager.AssertAllNodesHaveAuthorizedKey(new_node_key) >> >> def testAddKeyFailedOnOldNodeWithRetries(self): >> """Tests adding keys when updating one node's SSH setup fails. >> @@ -1572,9 +1562,8 @@ class >> TestAddRemoveGenerateNodeSshKey(testutils.GanetiTestCase): >> noded_cert_file=self.noded_cert_file, >> run_cmd_fn=self._run_cmd_mock) >> >> - self.assertTrue(self._ssh_file_manager.NoNodeHasPublicKey( >> - node_uuid, node_key)) >> - >> self.assertTrue(self._ssh_file_manager.NoNodeHasAuthorizedKey(node_key)) >> + self._ssh_file_manager.AssertNoNodeHasPublicKey(node_uuid, node_key) >> + self._ssh_file_manager.AssertNoNodeHasAuthorizedKey(node_key) >> >> def testRemoveKeyFailedWithRetriesOnOtherNode(self): >> """Test removing keys even if one of the old nodes fails even with >> retries. >> diff --git a/test/py/testutils_ssh.py b/test/py/testutils_ssh.py >> index a180144..43ba709 100644 >> --- a/test/py/testutils_ssh.py >> +++ b/test/py/testutils_ssh.py >> @@ -257,7 +257,7 @@ class FakeSshFileManager(object): >> """ >> return key in self._authorized_keys[file_node_name] >> >> - def AllNodesHaveAuthorizedKey(self, key): >> + def AssertAllNodesHaveAuthorizedKey(self, key): >> """Check if all nodes have a particular key in their auth. keys file. >> >> @type key: string >> @@ -270,9 +270,8 @@ class FakeSshFileManager(object): >> if key not in self._authorized_keys[name]: >> raise Exception("Node '%s' does not have the key '%s' in its" >> " 'authorized_keys' file." % (name, key)) >> - return True >> >> - def NoNodeHasAuthorizedKey(self, key): >> + def AssertNoNodeHasAuthorizedKey(self, key): >> """Check if none of the nodes has a particular key in their auth. >> keys file. >> >> @type key: string >> @@ -286,9 +285,8 @@ class FakeSshFileManager(object): >> if key in node_auth_keys: >> raise Exception("Node '%s' does have the key '%s' in its" >> " 'authorized_keys' file." % (name, key)) >> - return True >> >> - def NoNodeHasPublicKey(self, uuid, key): >> + def AssertNoNodeHasPublicKey(self, uuid, key): >> """Check if none of the nodes have the given public key in their >> file. >> >> @type uuid: string >> @@ -302,9 +300,8 @@ class FakeSshFileManager(object): >> if (uuid, key) in node_pub_keys.items(): >> raise Exception("Node '%s' does have public key '%s' of node >> '%s'" >> % (name, key, uuid)) >> - return True >> >> - def PotentialMasterCandidatesOnlyHavePublicKey(self, query_node_name): >> + def AssertPotentialMasterCandidatesOnlyHavePublicKey(self, >> query_node_name): >> """Checks if the node's key is on all potential master candidates >> only. >> >> This ensures that the node's key is in all public key files of all >> -- >> 2.2.0.rc0.207.ga3a616c >> >> Hrvoje Ribicic Ganeti Engineering Google Germany GmbH Dienerstr. 12, 80331, München Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Geschäftsführer: Graham Law, Christine Elizabeth Flores Steuernummer: 48/725/00206 Umsatzsteueridentifikationsnummer: DE813741370
