Nir Soffer has posted comments on this change. Change subject: VolumeMetadata: Add from_lines factory method ......................................................................
Patch Set 4: Code-Review-1 (11 comments) 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 that this test must be more specific about the required keys. 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 "FORMAT=format". test_from_lines_invalid_size? Also FOO is less clear than NOT_AN_INT This also does not test that invalid ctime or mtime raise. We should be more specific about the behavior of such infrastructure. 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. 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 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 legacy name. 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 value or raise ValueError if it cannot convert the string. 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 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". 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: if "=" not in line: continue 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! 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: md = {} for line in lines: if line.startswith("EOF"): break if "=" not in line: continue key, value = line.split("=", 1) md[key.strip()] = value.strip() try: # Here we do the mapping and the validation return cls(sd_id = md[DOMAIN], img_id = md[IMAGE], parent_vol_id = md[PUUID], size = int(md[SIZE]), vol_format = md[FORMAT], alloc_policy = md[PREALOCATE], parental_status = md[VOLTYPE], disk_type = md[DISKTYPE], description = md[DESCRIPTION], legality = md[LEGALITY], ctime = int(md[CTIME]), mtime = int(md[MTIME])) except KeyError as e: raise se.MetaDataKeyNotFoundError("Missing metadata key: %s: " "found: %s" % (e, md)) Notes: - ignore POOL without writing any code for it - don't warn about unknown keys, we don't care about it, and it is less forward compatible - don't show all missing keys, but do we care about it? - MD_FIELDS not needed 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
