Adam Litke has posted comments on this change. Change subject: VolumeMetadata: Add from_lines factory method ......................................................................
Patch Set 4: (12 comments) https://gerrit.ovirt.org/#/c/52672/3//COMMIT_MSG Commit Message: Line 8: Line 9: Add support for creating a VolumeMetadata instance from a list of lines. Line 10: This is the format that can be read from either block or file based Line 11: volume metadata and enables us to consolidate the logic into one place. Line 12: > Add comment that we now raise when integer values were expected in from_lin Done Line 13: Note the following semantics changes: Line 14: Line 15: - from_lines can raise ValueError if fields that represent int values Line 16: are not valid integers https://gerrit.ovirt.org/#/c/52672/4/tests/storage_volume_metadata_test.py File tests/storage_volume_metadata_test.py: Line 107: params = get_params() Line 108: params[param] = 'not_an_int' Line 109: self.assertRaises(AssertionError, volume.VolumeMetadata, **params) Line 110: Line 111: def test_from_lines_missing_param(self): > Missing POOL will not raise, but this test does not check this. I think tha Agreed. Split the pool test case out and am now testing the omission of each required key as a separate permutation. Line 112: self.assertRaises(se.MetaDataKeyNotFoundError, Line 113: volume.VolumeMetadata.from_lines, []) Line 114: Line 115: def test_from_lines_invalid_param(self): Line 118: lines.insert(0, "FOO=BAR") Line 119: self.assertNotIn("FOO", Line 120: volume.VolumeMetadata.from_lines(lines).legacy_info()) Line 121: Line 122: def test_from_lines_invalid_format(self): > invalid_format is confusing, I was trying to understand whats wrong with "F renamed and test all int params via permutations. Line 123: lines = ["DOMAIN=domain", "IMAGE=image", "PUUID=parent", "SIZE=FOO", Line 124: "FORMAT=format", "TYPE=type", "VOLTYPE=voltype", Line 125: "DISKTYPE=disktype", "EOF"] Line 126: self.assertRaises(ValueError, Line 121: Line 122: def test_from_lines_invalid_format(self): Line 123: lines = ["DOMAIN=domain", "IMAGE=image", "PUUID=parent", "SIZE=FOO", Line 124: "FORMAT=format", "TYPE=type", "VOLTYPE=voltype", Line 125: "DISKTYPE=disktype", "EOF"] > This is very hard to read, please use always one item per line, sorted. done. Added a helper. Line 126: self.assertRaises(ValueError, Line 127: volume.VolumeMetadata.from_lines, lines) Line 128: Line 129: def test_from_lines(self): Line 129: def test_from_lines(self): Line 130: lines = ["DOMAIN=domain", "IMAGE=image", "PUUID=parent", "SIZE=0", Line 131: "FORMAT=format", "TYPE=type", "VOLTYPE=voltype", Line 132: "DISKTYPE=disktype", "DESCRIPTION=description", "MTIME=0", Line 133: "CTIME=0", "LEGALITY=legality", "EOF"] > Same Done Line 134: md = volume.VolumeMetadata.from_lines(lines) Line 135: self.assertEqual('domain', md.sd_id) Line 136: self.assertEqual('image', md.img_id) Line 137: self.assertEqual('parent', md.parent_vol_id) https://gerrit.ovirt.org/#/c/52672/4/vdsm/storage/volume.py File vdsm/storage/volume.py: Line 199 Line 200 Line 201 Line 202 Line 203 > Lets use validate_description - this class is does not have any other legac Done Line 169: TYPE_NETWORK = "network" Line 170: Line 171: Line 172: class VolumeMetadata(object): Line 173: # metadata_key: (attribute, coercion_fn) > This is not coercion but validation - it convert the string to the correct Done Line 174: MD_FIELDS = { Line 175: DOMAIN: ('sd_id', None), Line 176: IMAGE: ('img_id', None), Line 177: PUUID: ('parent_vol_id', None), Line 209: Line 210: @classmethod Line 211: def from_lines(cls, lines): Line 212: kwargs = {} Line 213: found_md = [] > We can use set() and name it found_keys Done Line 214: for l in lines: Line 215: if l.startswith("EOF"): Line 216: break Line 217: if l.find("=") < 0: Line 210: @classmethod Line 211: def from_lines(cls, lines): Line 212: kwargs = {} Line 213: found_md = [] Line 214: for l in lines: > Use "line" instead of "l". Done Line 215: if l.startswith("EOF"): Line 216: break Line 217: if l.find("=") < 0: Line 218: continue Line 213: found_md = [] Line 214: for l in lines: Line 215: if l.startswith("EOF"): Line 216: break Line 217: if l.find("=") < 0: > Better use: Done Line 218: continue Line 219: key, value = [item.strip() for item in l.split("=", 1)] Line 220: Line 221: if key == POOL: # POOL is deprecated Line 230: found_md.append(key) Line 231: Line 232: missing = set(cls.MD_FIELDS.keys()) - set(found_md) Line 233: if missing: Line 234: raise se.MetaDataKeyNotFoundError("Missing: %s" % list(missing)) > Nice! thanks :) Line 235: return cls(**kwargs) Line 236: Line 237: @property Line 238: def description(self): Line 231: Line 232: missing = set(cls.MD_FIELDS.keys()) - set(found_md) Line 233: if missing: Line 234: raise se.MetaDataKeyNotFoundError("Missing: %s" % list(missing)) Line 235: return cls(**kwargs) > I think we can simplify this a lot like this: Nice! Line 236: Line 237: @property Line 238: def description(self): Line 239: return self._description -- To view, visit https://gerrit.ovirt.org/52672 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id7f63dce8e42e99d3240f4e916a9bd6ee5ee4b61 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke <ali...@redhat.com> Gerrit-Reviewer: Adam Litke <ali...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches