Include lifecycle policy tests by sratcliffe · Pull Request #239 · ska-sa/katdal

sratcliffe · 2019-04-24T13:54:18Z

We can't test expiry directly, firstly because minio doesn't support lifecycle polices, and secondly because the minimum expiry time is 1 day.

This basically just test the infrastructure and that the schema for the policy is created correctly.

We can't test expiry directly, firstly because minio doesn't support lifecycle polices, and secondly because the minimum expiry time is 1 day. This basically just test the infrastructure and that the schema for the policy is created correctly.

ludwigschwardt

Still need to understand the use case a bit better...

ludwigschwardt · 2019-05-02T12:08:49Z

katdal/schemas/__init__.py

+            if `xml_string` cannot be parsed into a valid XML document
+        """
+        try:
+            xml_doc = etree.fromstring(bytes(bytearray(xml_string, encoding='utf-8')))


Out of curiosity, why bytes and bytesarray?

Python2.7 doesn't allow encoding types in bytes, but does in bytearray. Kludge to keep 2/3 compatible.

There should be something in future.utils to force things one way or another.

But also, why encode at all? lxml.etree.fromstring seems perfectly happy to take Unicode.

ludwigschwardt · 2019-05-02T12:09:57Z

katdal/schemas/__init__.py

+        try:
+            xml_doc = etree.fromstring(bytes(bytearray(xml_string, encoding='utf-8')))
+        except etree.XMLSyntaxError as e:
+            raise ValueError(e)


This is a candidate for raise_from.

ludwigschwardt · 2019-05-02T12:12:16Z

katdal/schemas/__init__.py

+            raise ValueError(e)
+        if not self.validator.validate(xml_doc):
+            log = self.validator.error_log
+            raise etree.DocumentInvalid(log.last_error)


You are converting etree.XMLSyntaxError to ValueError but leaving etree.DocumentInvalid as is... This seems a bit inconsistent. Will the end user typically be expected to catch invalid document exceptions? If that's the case, I'd probably also transmogrify it for the user's convenience.

Simply because the XMLSyntaxError is quite messy. I also wanted to easily catch the difference between an XML syntax error and an invalid document. If I didn't use DocumentInvalid, then ValueError would be the sensible standard exception but wouldn't allow this differentiation.

ludwigschwardt · 2019-05-02T12:13:34Z

katdal/schemas/__init__.py

+    try:
+        return validators[validator_name].validate(string_to_validate)
+    except KeyError:
+        raise KeyError("Specified validator {} doesn't map to an installed"


Maybe raise_from(..., None) here to suppress the double KeyError in the chain.

ludwigschwardt · 2019-05-02T12:17:18Z

test-requirements.txt

@@ -1,5 +1,6 @@
 coverage
 funcsigs
+lxml==4.3.3


Do you want this in requirements.txt as well, to be used on site?

Nope - the import checking handles this being missing for production systems and lets you know that validation wont be used if it is missing.

ludwigschwardt · 2019-05-02T12:20:50Z

katdal/chunkstore.py

    """Could not access underlying storage medium (offline, auth failed, etc)."""


+class NotSupported(ChunkStoreError):


This name is a bit vague... How about UnsupportedStoreFeature?

Fair enough - changed

ludwigschwardt · 2019-05-02T12:23:20Z

katdal/chunkstore_s3.py

            xml_payload = _BASE_LIFECYCLE_POLICY.format(self.expiry_days)
+            if self.validate_xml_policies:
+                if not schemas.has_lxml:
+                    raise ImportError("XML schema validation requires lxml to be installed.")


It would be nice to link to the original error via raise_from. See e.g. the RDB reader handling in katsdptelstate.

Not sure I agree - I prefer the very unambiguous nature of just telling the user that you need lxml if you want to use validators.

ludwigschwardt · 2019-05-02T12:28:09Z

katdal/chunkstore_s3.py

@@ -467,6 +483,10 @@ def create_array(self, array_name):

        if self.expiry_days > 0:
            xml_payload = _BASE_LIFECYCLE_POLICY.format(self.expiry_days)


So xml_payload is always _BASE_LIFECYCLE_POLICY? When would it not validate - a bad value for expiry_days? It seems like a lot of effort to solve future problems.

The grand scheme is that you will sometimes want to override the base for very user specific things - like managing storage migration say. I can see us ending up with quite a number of policies as future versions of CEPH support more elaborate management via lifecycle policies.

sratcliffe added 2 commits April 24, 2019 15:41

Included tests for bucket expiry.

8108360

We can't test expiry directly, firstly because minio doesn't support lifecycle polices, and secondly because the minimum expiry time is 1 day. This basically just test the infrastructure and that the schema for the policy is created correctly.

Flake8 and docstrings

156309f

sratcliffe requested a review from ludwigschwardt April 24, 2019 13:54

sratcliffe and others added 4 commits April 24, 2019 16:59

Allow switchable validation to avoid hard lxml dependency

44cb6e9

Pin lxml version

b0216df

Generalise validator a little

1482ccf

Include package data for schemas

7e0ffd6

ludwigschwardt requested changes May 2, 2019

View reviewed changes

PR fixes - mostly around exception naming and handling

ac78f20

		"""Could not access underlying storage medium (offline, auth failed, etc)."""


		class NotSupported(ChunkStoreError):

		@@ -467,6 +483,10 @@ def create_array(self, array_name):

		if self.expiry_days > 0:
		xml_payload = _BASE_LIFECYCLE_POLICY.format(self.expiry_days)

Comments

Conversation

sratcliffe commented Apr 24, 2019

Uh oh!

ludwigschwardt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants