Change in vdsm[master]: VolumeMetadata: Add from_lines factory method
alitke at redhat.com
alitke at redhat.com
Thu Apr 21 14:42:03 UTC 2016
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 <alitke at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes
More information about the vdsm-patches
mailing list