673036 - Clean up data handling in AUS3

Status: RESOLVED DUPLICATE of bug 1021018

(Release Engineering Graveyard :: Applications: Balrog (backend), defect, P2)

Description

[Nick Thomas [:nthomas] (UTC+12)]

Getting from JSON data to update.xml (via expandRelease and createXML) is more complicated than it needs to be, and creates intermediate data without a great deal of utility. Time for a refactor and streamline.

Comment 1

[[e-mail] (:bhearsum)]

There's a chance that some of this will get addressed in bug 748698, but I want to make sure these ideas aren't lost if they aren't:
+def createBlob(data):
+    """Takes a string form of a blob (eg from DB or API) and converts into an
+    actual blob, taking care to notice the schema"""
+    data = json.loads(data)
+    try:
+        if data['schema_version'] == 1:
+            return ReleaseBlobV1(data)
+        elif data['schema_version'] == 2:
+            return ReleaseBlobV2(data)
+    except KeyError, e:
+        raise ValueError("schema_version is not set")
+

What do you think about having createBlob handle adding schema_version to the blob if it doesn't exist? That'd let us get rid of all of the CURRENT_SCHEMA_VERSION stuff from releases.py/forms.py (until we have a use case for creating older versions of blobs, that is). This could potentially go in the base Blob if there was a schema_version class variable, too.


+        elif relData['schema_version'] == 2:
+            updateData['displayVersion'] = relData.getDisplayVersion(buildTarget, locale)
+            updateData['appVersion'] = relData.getAppVersion(buildTarget, locale)
+            updateData['platformVersion'] = relData.getPlatformVersion(buildTarget, locale)
+            for attr in self.SCHEMA_2_OPTIONAL_ATTRIBUTES:
+                if attr in relData:
+                    updateData[attr] = relData[attr]
+
+        # general properties
+        updateData['type'] = update_type
+        updateData['schema_version'] = relData['schema_version']
+        updateData['build'] = relData.getBuildID(buildTarget, locale)
+        if 'detailsUrl' in relData:
+            updateData['detailsUrl'] = relData['detailsUrl'].replace('%LOCALE%',updateQuery['locale'])
+        if 'licenseUrl' in relData:
+            updateData['licenseUrl'] = relData['licenseUrl'].replace('%LOCALE%',updateQuery['locale'])

I haven't actually tried to see if it will work, but it seems like the retrieval of all this update data might be a good fit to go in the Blob classes (including the locale-specific retrievals down below). The interface could be something like 'def getUpdateData(self, platform, locale)'. If that worked, you'd be able to get rid of some of the intermediary variables here, like relDataPlatLoc; it also may make this code more testable. This would probably require moving SCHEMA_2_OPTIONAL_ATTRIBUTES into that blob, too. That also might be better off investigating as part of https://bugzilla.mozilla.org/show_bug.cgi?id=673036. Definitely not a blocker for this bug.


+                        "displayVersion=%s" % rel['displayVersion'],
+                        "appVersion=%s" % rel['appVersion'],
+                        "platformVersion=%s" % rel['platformVersion']
+                        ]
+            if rel['detailsUrl']:
+                snippet.append("detailsUrl=%s" % rel['detailsUrl'])
+            if rel['licenseUrl']:
+                snippet.append("licenseUrl=%s" % rel['licenseUrl'])
+            if rel['type'] == 'major':
+                snippet.append('updateType=major')
+            for attr in self.SCHEMA_2_OPTIONAL_ATTRIBUTES:
+                if attr in rel:
+                    snippet.append('%s=%s' % (attr, rel[attr]))
+            # AUS2 snippets have a trailing newline, add one here for easy diffing
+            snippets[patch['type']] = "\n".join(snippet) + '\n'
+

Related to my comment about retrieving update data from above...I wonder if this belongs in or nearer the blobs too. The more I see the schema version specific stuff in multiple places, the more it feels like we should have a common interface for all schemas that this class can access. Again, that might be something for bug 673036 or elsewhere.


All of the code above is from https://github.com/nthomas-mozilla/balrog/blob/2d814d1a974888977ebe615b115fc528146d19b0/auslib/AUS.py.

Comment 2

[[e-mail] (:bhearsum)]

mass component change

Comment 3

[[e-mail] (:bhearsum)]

This might end up getting fixed as part of bug 1021018.

Comment 4

[[e-mail] (:bhearsum)]

(In reply to Ben Hearsum [:bhearsum] from comment #3)
> This might end up getting fixed as part of bug 1021018.

Nick and I spoke on IRC and agreed this should be fine.