Newer attributes #14

Closed
wants to merge 41 commits into
from

Conversation

Projects
None yet
2 participants
Member

mozbhearsum commented Jun 21, 2013

Creating a pull request for this because it seems that I can't comment on the diff otherwise.

@mozbhearsum mozbhearsum and 1 other commented on an outdated diff Jun 21, 2013

...napshots/xml-attributes-v2/Firefox-13.0.1-build1.json
+ "detailsUrl": "https://www.mozilla.com/%LOCALE%/firefox/13.0.1/releasenotes/",
+ "fileUrls": {
+ "betatest": "http://stage.mozilla.org/pub/mozilla.org/firefox/nightly/13.0.1-candidates/build1/update/%OS_FTP%/%LOCALE%/%FILENAME%",
+ "beta": "http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/13.0.1-candidates/build1/update/%OS_FTP%/%LOCALE%/%FILENAME%",
+ "releasetest": "http://download.mozilla.org/?product=%PRODUCT%&os=%OS_BOUNCER%&lang=%LOCALE%",
+ "release": "http://download.mozilla.org/?product=%PRODUCT%&os=%OS_BOUNCER%&lang=%LOCALE%"
+ },
+ "ftpFilenames": {
+ "partial": "firefox-13.0-13.0.1.partial.mar",
+ "complete": "firefox-13.0.1.complete.mar"
+ },
+ "bouncerProducts": {
+ "partial": "firefox-13.0.1-partial-13.0",
+ "complete": "firefox-13.0.1-complete"
+ },
+ "hashFunction": "SHA512",
@mozbhearsum

mozbhearsum Jun 21, 2013

Member

Technically it doesn't matter in these tests, but we've been using lowercase sha512 for everything since https://bugzilla.mozilla.org/show_bug.cgi?id=853036.

@mozbhearsum mozbhearsum commented on an outdated diff Jun 21, 2013

auslib/blob.py
@@ -35,6 +35,18 @@ def isValidBlob(format_, blob):
return False
return True
+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")
+
@mozbhearsum

mozbhearsum Jun 21, 2013

Member

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.

@mozbhearsum mozbhearsum commented on the diff Jun 21, 2013

auslib/AUS.py
+ 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'])
@mozbhearsum

mozbhearsum Jun 21, 2013

Member

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.

@mozbhearsum mozbhearsum commented on an outdated diff Jun 21, 2013

auslib/AUS.py
- updateData['patches'].append(patch)
+ url += '?force=1'
+
+ updateData['patches'].append({
+ 'type': patchKey,
+ 'URL': url,
+ 'hashFunction': relData['hashFunction'],
+ 'hashValue': patch['hashValue'],
+ 'size': patch['filesize']
+ })
+ else:
+ self.log.debug("Didn't add patch for patchKey '%s'; from is '%s', updateQuery name is '%s'", patchKey, patch['from'], updateQuery['name'])
+
+ # older branches required a <partial> in the update.xml, which we
+ # used to fake by repeating the complete data.
+ if 'fakePartials' in relData and relData['fakePartials'] and len(updateData['patches']) == 1 and \
@mozbhearsum

mozbhearsum Jun 21, 2013

Member

Are we far enough removed from this to just kill fakePartials with fire?

@mozbhearsum mozbhearsum commented on the diff Jun 21, 2013

auslib/AUS.py
+ "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'
+
@mozbhearsum

mozbhearsum Jun 21, 2013

Member

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.

@mozbhearsum mozbhearsum commented on the diff Jun 21, 2013

auslib/blob.py
@@ -35,6 +35,18 @@ def isValidBlob(format_, blob):
return False
return True
+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:
@mozbhearsum

mozbhearsum Jun 21, 2013

Member

Since schema_version is required it might be good to check for it as part of the isValid method of a Blob.

@mozbhearsum mozbhearsum and 1 other commented on an outdated diff Jun 21, 2013

auslib/test/test_AUS.py
@@ -4,7 +4,7 @@
import unittest
@mozbhearsum

mozbhearsum Jun 21, 2013

Member

You were wondering if you had enough tests for this code, and I think you do...at least, I can't think of any additional useful tests that could add. All of the new attributes are simply strings that are copied into the snippet or XML, so it's not like there's any special behaviour for any of them that needs to get tested.

@nthomas-mozilla

nthomas-mozilla Jun 24, 2013

Contributor

Lots of good comments to look at here. A few quick replies

  • I like moving snippet/XML creation out to functions on the blobs
  • not sure about having the server side defaulting to a particular schema
    version, might be better to insist the submitter send it to avoid
    consistency issues
  • fake partials we probably have to keep for old update paths, but maybe
    only in the v1 blob

In auslib/test/test_AUS.py:

@@ -4,7 +4,7 @@
import unittest

You were wondering if you had enough tests for this code, and I think you
do...at least, I can't think of any additional useful tests that could add.
All of the new attributes are simply strings that are copied into the
snippet or XML, so it's not like there's any special behaviour for any of
them that needs to get tested.


Reply to this email directly or view it on
GitHubhttps://github.com/mozilla/balrog/pull/14/files#r4822868
.

nthomas-mozilla added some commits Jun 26, 2013

@nthomas-mozilla nthomas-mozilla Use sha512 instead of SHA512 42052e3
@nthomas-mozilla nthomas-mozilla Blobs must have a schema_version set to an int (if it's in the format…
…), and make sure schema_version is set on direct initialisation of a blob (rather than via createBlob, which now fails if it's not set)
bfbf8aa
@nthomas-mozilla nthomas-mozilla Merge master (domain whitelist + backgroundRate) with fixes 09ef02f
@nthomas-mozilla nthomas-mozilla Trim README down 717621d
@nthomas-mozilla nthomas-mozilla Fix merge state 8a9f620
@nthomas-mozilla nthomas-mozilla Merge branch 'master' into newer-attributes e5e47d5
@nthomas-mozilla nthomas-mozilla Merge branch 'master' into newer-attributes d83561c
@nthomas-mozilla nthomas-mozilla Handle requests from buildTarget's with no update, and don't error wh…
…en wrong request to use partial
123199a
@nthomas-mozilla nthomas-mozilla Fix mounting of code for newer vagrant c98efaf
@nthomas-mozilla nthomas-mozilla Merge from upstream/master 44f7d14
@nthomas-mozilla nthomas-mozilla Remove test debugging 485f800
@nthomas-mozilla nthomas-mozilla Support to submit different schemas via API, swap default blob to sch…
…ema_version 2
47ed04b
@nthomas-mozilla nthomas-mozilla No stinking fakePartials in the modern world 64dff86
@nthomas-mozilla nthomas-mozilla Minor tidy ups df10683
@nthomas-mozilla nthomas-mozilla Polishing 91a0f89
@nthomas-mozilla nthomas-mozilla Push v2 optional values onto blob property instead of AUS class c00bcad
@nthomas-mozilla nthomas-mozilla Remove wayward file 8946e0d
@nthomas-mozilla nthomas-mozilla More polish 8405b47
@nthomas-mozilla nthomas-mozilla Merge from master; update new tests, add blob.getApplicationVersion t…
…hat abstracts lower level versions
790dd6c
@nthomas-mozilla nthomas-mozilla Pyflakes, whitespace 37b3cde
@nthomas-mozilla nthomas-mozilla Merge bug 721233 from master 752c598
@nthomas-mozilla nthomas-mozilla Merge from master 83e43b4
@nthomas-mozilla nthomas-mozilla Merge from master cbce5fb
@nthomas-mozilla nthomas-mozilla Merge branch 'master' into newer-attributes a475c5a
@nthomas-mozilla nthomas-mozilla Merge branch 'master' into newer-attributes abfe689
@nthomas-mozilla nthomas-mozilla Revert default blob to 1 for graceful transfer, add some tests around…
… release submission
3cdb73f
@nthomas-mozilla nthomas-mozilla Add comments for getApplicationVersion d918ab1
@nthomas-mozilla nthomas-mozilla Merge branch 'master' into newer-attributes b8a706a

nthomas-mozilla deleted the nthomas-mozilla:newer-attributes branch May 30, 2014

@JohanLorenzo JohanLorenzo pushed a commit to JohanLorenzo/balrog that referenced this pull request Sep 9, 2016

@davemo davemo [balrog-ui] Merge pull request #14 from Foxandxss/patch-1
Let npm update protractor with new versions
6f41d3f

@JohanLorenzo JohanLorenzo pushed a commit to JohanLorenzo/balrog that referenced this pull request Sep 9, 2016

@bhearsum bhearsum [balrog-ui] Merge pull request #14 from bhearsum/rule-alias
Add support for rule alias'.
021460f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment