Closed
Bug 459972
Opened 16 years ago
Closed 13 years ago
Add new attributes to the update snippets
Categories
(AUS Graveyard :: API, defect)
Tracking
(firefox6-, firefox7+, blocking2.0 -)
VERIFIED
FIXED
People
(Reporter: robert.strong.bugs, Assigned: rhelmer)
References
Details
Attachments
(5 files, 4 obsolete files)
This would identify the platform version which is equivalent to the GRE version / Toolkit version.
Reporter | ||
Comment 1•16 years ago
|
||
Not sure if this is correct but thought I'd try to help get this fixed.
Reporter | ||
Comment 2•16 years ago
|
||
Hey morgamic, I know AMO isn't planning on supporting this anytime soon but other sites may want to and I'd like to get this in before 3.1
Reporter | ||
Updated•15 years ago
|
Summary: Add platformVersion to the update snippets → Add new attributes to the update snippets
Reporter | ||
Comment 3•15 years ago
|
||
displayVersion replaces version but version is still valid for backwards compatibility.
/**
* The string to display in the user interface for the version. If you want
* a real version number use appVersion.
*/
attribute AString displayVersion;
appVersion replaces extensionVersion but extensionVersion is still valid for
/**
* The Application version of this update.
*/
attribute AString appVersion;
Besides providing the url to the billboard this also controls whether to display the billboard.
/**
* The URL to a page that is typically localized to display in the update
* prompt.
*/
attribute AString billboardURL;
The custom actions based on the values of extr1 are being implemented in bug 538331 and possible values at this time are:
not specified in the update xml = This should be the current action using the pref containing the url.
silent = do nothing.
openURL = open a url. This should be the current action using the pref containing the url.
openURL http://etc. = open the url specified after the space.
showNotification = show the notification bar. The url should be the url contained in the current action pref.
showNotification http://etc. = show the notification bar with the url specified after the space.
showAlert = show an alert. The url should be the url contained in the current action pref.
showAlert = show an alert. The url should be the url contained in the current
showAlert http://etc. = show an alert with the url specified after the space.
/**
* Stores custom string data provided by the update xml for use by the
* application.
*
* Implementation Note: After an update has been successfully applied this
* value will be added to the app.update.extra preference and if an
* application uses this preference to determine that an update has been
* applied it should also delete the preference.
*/
attribute AString extra1;
We currently always show the prompt for major updates... this allows it to be configured in the update xml.
/**
* Whether to show the update prompt which requires user confirmation when an
* update is found during a background update check. This overrides the
* default setting to download the update in the background.
*/
attribute boolean showPrompt;
We currently always show the "No Thanks" button for major updates... this allows it to be configured in the update xml.
/**
* Whether to show the "No Thanks" button in the update prompt. This allows
* the user to never receive a notification for that specific update version
* again.
*/
attribute boolean showNeverForVersion;
I am going to add the ability to show an url for filling out a survey in the update prompt. The name / possible values might change for this one so if you can hold off on it I'd appreciate it. If not, then please implement it with the rest and I'll implement it as is.
/**
* Whether to show the survey link in the update prompt. The url must also be
* present in the app.update.surveyURL preference.
*/
attribute boolean showSurvey;
Reporter | ||
Comment 4•15 years ago
|
||
nthomas, I've added implementation details in comment #3. I also forgot to ask to please add platformVersion at the same time.
Reporter | ||
Comment 5•15 years ago
|
||
nthomas, just a heads up that bug 530872 should land fairly soon. Updates will still work with the current update snippets but to get the previous behavior using the overloaded values the new values will need to be used.
Reporter | ||
Comment 6•15 years ago
|
||
Adding Seamonkey folks so they get a heads up on this.
Comment 7•15 years ago
|
||
Robert, can you explain a bit more about this? Is this the work to improve the options available for updates? and is there some sort of spec on what we're going to be able to do?
(In reply to comment #5)
> Updates will
> still work with the current update snippets but to get the previous behavior
> using the overloaded values the new values will need to be used.
The way I read that, is that old trees are going to need to be modified for the updates to work the same. So will the Thunderbird 2 code or something(?) need changing to support this?
What is the order for patches landing in AUS, code base, patcher config(?) ?
Comment 8•15 years ago
|
||
(oh and what's also confusing is the patch attached to this bug seems to be against the tinderbox)
Reporter | ||
Comment 9•15 years ago
|
||
Adding dependency for reference though it doesn't really depend on it.
Mark, this mainly will allow apps to specify a string in an attribute that app update adds to a pref that the app can then read to determine that a) the app was just updated and b) perform an action based on the value (see bug 538331 for the Firefox client side work and bug 530872 for the app update work). Besides that it adds several individual attributes to the update xml that were overloaded which broke / limited functionality.
Depends on: 530872
Comment 10•15 years ago
|
||
As a note, the patch in here is probably obsolete, I don't think we're actually still using tinderbox client anywhere.
Comment 11•15 years ago
|
||
Comment on attachment 343157 [details] [diff] [review]
post-mozilla-rel.pl patch
We do still use tinderbox for Fx3.0 and Tb2.0, but I'll be astounded if we backport this sort of update change to zombie branches.
Attachment #343157 -
Attachment is obsolete: true
Reporter | ||
Comment 12•15 years ago
|
||
fyi: I've been asked to support localization of the UI through the update xml by beltzner. What I am planning on doing is adding support for saving all attributes locally with the possible exception of a couple of well known attributes that we don't want to be provided by the update. After I get this sorted I'll post what the attributes will be... as it stands now all of the attributes I have stated will stay the same.
Comment 13•15 years ago
|
||
BTW, does that needs any updates to the AUS server as well? If so, we need to deploy those on the community and MoMo AUS servers as well, probably.
Reporter | ||
Comment 14•15 years ago
|
||
note: everything in comment #3 is the same except for the removal of extra1. Bug 549969 made it so non-standard update xml attributes will be saved locally and can then be read by the application after an update.
For Firefox the following additional attributes are implemented:
actions - possible values are any combination of showURL, showNotification, and showAlert as well as silent which if present will show nothing even with any of the other values present. The values should be space delimited since that is what the tests use but it will work with other delimiters.
openURL - the url to open if showURL is in actions. If openURL is not present then the current url opened on version change will be opened.
Notification Box:
notificationText - self explanatory. A locale string will be used if not present.
notificationURL - the url to open if the notification button is clicked. If notificationURL is not present then the current url opened on version change will be opened if the notification button is clicked.
notificationButtonLabel - self explanatory. A locale string will be used if not present.
notificationButtonAccessKey - self explanatory. A locale string will be used if not present.
Alert Notification:
alertTitle - self explanatory. A locale string will be used if not present.
alertText - self explanatory. A locale string will be used if not present.
alertURL - the url to open if the alert is clicked. If alertURL is not present then the current url opened on version change will be opened if the alert is clicked.
The Firefox side was implemented in bug 538331 and landed on trunk on the April 6th.
Reporter | ||
Comment 15•15 years ago
|
||
btw: the current plan is to backport these changes to 1.9.2.5 / Firefox 3.6.5.
Comment 16•14 years ago
|
||
Ugh, I should have nominated this for blocking, and now it's late. Morgamic and rhelmer will let me know how much they hate me, and whether or not this is do-able.
blocking2.0: --- → ?
Comment 17•14 years ago
|
||
The snippets themselves support key/value pairs, but we'd have to modify the xml output to display these fields, which is pretty simple if the data is there.
Comment 18•14 years ago
|
||
Pardon my ignorance but, where does the data come from? Or is that something that needs to be generated by the RelEng gents (cc'd)
Comment 19•14 years ago
|
||
Could be manual, if that's what we really need to do for releases. That is testable on beta/test channels before hitting primetime of course.
Reporter | ||
Comment 20•14 years ago
|
||
btw: I could add code to fallback to the previous behavior if one of the new attributes that should be in use isn't specified. For example, if appVersion isn't specified then fallback.
Comment 21•14 years ago
|
||
(In reply to comment #18)
> Pardon my ignorance but, where does the data come from? Or is that something
> that needs to be generated by the RelEng gents (cc'd)
Yeah, our tools would need to know how to populate the snippets with this data. As Morgamic mentioned, it could be done manually, but if we're talking about using them on a regular basis it'd behoove us to add support in the tools.
Comment 22•14 years ago
|
||
Mike - what's your timeline?
Reporter | ||
Comment 23•14 years ago
|
||
Files bug 599274 to fallback to the previous behavior when the update snippet doesn't contain one of the new properties (most likely appVersion).
Speaking for myself regarding the timeline, the sooner the better as in prior to Firefox 4.0 release.
Comment 24•14 years ago
|
||
Yeah, timeline = pronto, but if you need a tighter deadline than that, I think we need to test this in the beta period, so mid-October would be nice.
Updated•14 years ago
|
blocking2.0: ? → -
Whiteboard: [d?]
Comment 25•14 years ago
|
||
Removing [d?] - I agree with the non-blocking decision here, but want to re-register that this is a very-much-needed change to AUS in order to provide a much better set of behaviours for users, post-update-installation.
Whiteboard: [d?]
Comment 26•13 years ago
|
||
Can we get this back on the radar? I'm told this is needed to get rid of the annoying post-update tab.
Reporter | ||
Comment 27•13 years ago
|
||
For clarification, this will provide the option in the update snippet whether to display the post-update tab.
Comment 28•13 years ago
|
||
Adding to tracking for Firefox 7 and nominating for 6 to get this onto the release team's radar. This falls into the bucket of things we'd like to do to make the update a more streamlined experience now that it's happening at a much swifter cadence.
tracking-firefox6:
--- → ?
tracking-firefox7:
--- → +
Comment 29•13 years ago
|
||
Rob, where are these extra attributes going to be set? Are they going to change very much between versions?
Reporter | ||
Comment 30•13 years ago
|
||
The update snippet. Historically we haven't made many changes and I've tried to do them all at once so they shouldn't change often. Of course product drivers may come up with new requirements which require new attributes and it now allows free form attributes to be used by products without app update's needing to change such as comment #14.
Comment 31•13 years ago
|
||
(In reply to comment #30)
> The update snippet. Historically we haven't made many changes and I've tried
> to do them all at once so they shouldn't change often. Of course product
> drivers may come up with new requirements which require new attributes and
> it now allows free form attributes to be used by products without app
> update's needing to change such as comment #14.
Sorry, what I meant was, what is the original source of the data? AUS needs to include these in the update snippet, so that means AUS needs to be looking them up somewhere. Are they defined somewhere in the product? Do release/product drivers determine them for upcoming releases?
Reporter | ||
Comment 32•13 years ago
|
||
The ones from comment #3 are from
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/nsIUpdateService.idl#122
The ones from comment #14 are free form and are located here
http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js#624
I don't think AUS looks up the existing attributes that it uses and are defined in the app update code though.
Comment 33•13 years ago
|
||
We're not going to track this for 6 as the reason we'd need to get this in (differentiating service packs for a new compiler) isn't going to come until Firefox 8. Because we are already tracking this for Firefox 7 we don't need to track this for Firefox 6.
Comment 34•13 years ago
|
||
(In reply to comment #33)
> We're not going to track this for 6 as the reason we'd need to get this in
> (differentiating service packs for a new compiler) isn't going to come until
> Firefox 8. Because we are already tracking this for Firefox 7 we don't need
> to track this for Firefox 6.
So now we're on Aurora for Firefox 7, what's the state here?
Comment 35•13 years ago
|
||
(In reply to comment #34)
> So now we're on Aurora for Firefox 7, what's the state here?
Who's working on this? The assignee field says nobody.
Comment 36•13 years ago
|
||
Just had a conversation with Chris AtLee about this. I said:
- that I believed all the client work necessary around this was done (long done, in some cases)
- that this work wouldn't *block* a Firefox 7 go decision, but not having it for 7 would mean potentially pushing other work back in Firefox 8 since we need these pieces in place first, hence the tracking-firefox7+ even though it's not specific to changes in FF7
Chris said:
- There was a lack of clarity as to why it was tracking-firefox7 and he wasn't sure if that flag was supposed to mean "do this right now" or just "on radar" or somewhere in between.
- The ownership here is fuzzy, but he can make the changes happen on the AUS side now that he understands their importance more
So, in terms of making it go:
- Chris said that it would help size the work to understand whether any of the properties listed above (particularly ones likely to be thorny) can be moved to a follow-up in order to get the most important bits in now.
- He also said that supporting properties which are locale-dependent or otherwise computed is harder than supporting properties which are just fixed for a given update, so it would help to understand when we're likely to ask for the former vs. the latter.
I suggested that Robert Strong and Nick Thomas could probably knock those questions into an implementation plan in about 5 minutes, since they are closest to the code on client and aus respectively.
Gentlemen, thoughts?
Reporter | ||
Comment 37•13 years ago
|
||
The following are not locale specific and I don't think any of these are thorny.
displayVersion replaces version but version is still valid for backwards compatibility.
/**
* The string to display in the user interface for the version. If you want
* a real version number use appVersion.
*/
attribute AString displayVersion;
appVersion replaces extensionVersion but extensionVersion is still valid for
/**
* The Application version of this update.
*/
attribute AString appVersion;
Besides providing the url to the billboard this also controls whether to display the billboard.
/**
* The URL to a page that is typically localized to display in the update
* prompt.
*/
attribute AString billboardURL;
We currently always show the prompt for major updates... this allows it to be configured in the update xml.
/**
* Whether to show the update prompt which requires user confirmation when an
* update is found during a background update check. This overrides the
* default setting to download the update in the background.
*/
attribute boolean showPrompt;
We currently always show the "No Thanks" button for major updates... this allows it to be configured in the update xml.
/**
* Whether to show the "No Thanks" button in the update prompt. This allows
* the user to never receive a notification for that specific update version
* again.
*/
attribute boolean showNeverForVersion;
I am going to add the ability to show an url for filling out a survey in the update prompt. The name / possible values might change for this one so if you can hold off on it I'd appreciate it. If not, then please implement it with the rest and I'll implement it as is.
/**
* Whether to show the survey link in the update prompt. The url must also be
* present in the app.update.surveyURL preference.
*/
attribute boolean showSurvey;
For Firefox the following additional custom attributes are implemented:
actions - possible values are any combination of showURL, showNotification, and showAlert as well as silent which if present will show nothing even with any of the other values present. The values should be space delimited since that is what the tests use but it will work with other delimiters.
Content Window:
openURL - the url to open if showURL is in actions. If openURL is not present then the current url opened on version change will be opened.
Notification Box:
notificationURL - the url to open if the notification button is clicked. If notificationURL is not present then the current url opened on version change will be opened if the notification button is clicked.
Alert Notification:
alertURL - the url to open if the alert is clicked. If alertURL is not present then the current url opened on version change will be opened if the alert is clicked.
Reporter | ||
Comment 38•13 years ago
|
||
The following require localized strings and should likely be added later.
Notification Box:
notificationText - self explanatory. A locale string will be used if not present.
notificationButtonLabel - self explanatory. A locale string will be used if not present.
notificationButtonAccessKey - self explanatory. A locale string will be used if not present.
Alert Notification:
alertTitle - self explanatory. A locale string will be used if not present.
alertText - self explanatory. A locale string will be used if not present.
Reporter | ||
Comment 39•13 years ago
|
||
Also, platformVersion should be added. This is the Gecko version and is used for extension compatibility checks that use platformVersion for compatibility with all Mozilla apps instead of listing each app individually.
Reporter | ||
Comment 40•13 years ago
|
||
The default values for each property should be adequate for most updates. The following new properties should be provided for each update:
displayVersion
appVersion
platformVersion
LegNeato, could you take a look at the new properties in comment #37 to double check everything is to your liking? Especially regarding the new actions property... I personally think that should have a default value of either silent or openURL.
Reporter | ||
Comment 41•13 years ago
|
||
In case it isn't clear, when I say default values I am referring to what the client defaults to when the property isn't present.
Reporter | ||
Comment 42•13 years ago
|
||
Reporter | ||
Comment 43•13 years ago
|
||
Reporter | ||
Comment 44•13 years ago
|
||
I'll talk with LegNeato to find out which of the values are less important.
note: the values for actions on everything.xml show all possible values per:
actions - possible values are any combination of showURL, showNotification, and showAlert as well as silent which if present will show nothing even with any of the other values present. The values should be space delimited since that is what the tests use but it will work with other delimiters.
Reporter | ||
Comment 45•13 years ago
|
||
Talked with LegNeato and we came up with the following minimal set of new attributes
displayVersion
appVersion
platformVersion
billboardURL - (optional value)
showPrompt - (optional value)
showNeverForVersion - (optional value)
actions - (optional value) with a value of silent
optional value means that it should not be present except when the associated behavior should not occur.
Comment 46•13 years ago
|
||
re: list of attributes in comment 45.
If we ever wanted to:
1) announce a major feature that temporary didn't get translated in a specific locale build
or 2) ever needed to discontinue support for a locale due to the lack of contributors
would we also need some additional locale information?
Or is locale detection covered in one of these attributes, or available with another mechanism?
Comment 47•13 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #35)
> (In reply to comment #34)
> > So now we're on Aurora for Firefox 7, what's the state here?
>
> Who's working on this? The assignee field says nobody.
Four weeks left for Firefox 7. catlee, are you working on this?
Comment 48•13 years ago
|
||
Over in bug 682805 I have a patch to our utility that generates snippets. It specifies
displayVersion
appVersion
platformVersion
and the following are optional
billboardURL
showPrompt
showNeverForVersion
actions
openURL
notificationURL
alertURL
showSurvey
So that's a superset of comment #45, but turns out it's trivial to the last four when you have the machinery for the others. It doesn't support the localised strings in comment #38, which we have already agreed we'll defer till later.
There is a tarball of snippets over there which may be useful for testing changes in AUS.
Unlike details and license, we're intending to use exactly the same string (eg billboardURL) in the utility config, snippets, and then xml. So AUS is just passing through the new attributes, but at the moment I don't think it's switching the xml output based on the versioning of the snippets.
Comment 49•13 years ago
|
||
Talked with catlee and we agreed the AUS side of this can be done in time for 8 going into beta, that is, by September 27.
Assignee: nobody → laura
Comment 51•13 years ago
|
||
Is this still on track? We've got a lot more attention on it and want to make sure we don't wait another cycle for it.
Comment 52•13 years ago
|
||
Should be.
Comment 53•13 years ago
|
||
(In reply to Christian Legnitto [:LegNeato] from comment #51)
> Is this still on track? We've got a lot more attention on it and want to
> make sure we don't wait another cycle for it.
per comment 49, we're targeting the beta cycle for 8; we're going to miss the 7 train.
Assignee | ||
Comment 54•13 years ago
|
||
Exposes new attributes specified in "snippet" text files by bug 682805
I'm still working out the test matrix for the automated acceptance tests, looks good in my manual testing so far, and doesn't break any existing tests.
Some notes about this:
* doesn't look like bug 682805 supports nightlies, so didn't touch that code path in AUS
* the "version" and "extensionVersion" settings are hardcoded and baked in deeper than I would've expected
In general there are lots of assumptions that there is an "old schema" and "new schema" for snippets, which of course breaks down now that there are 3 schema versions :)
"hasUpdateInfo()" is another place where this assumption is baked in. There should instead be a way to determine the current snippet schema version, instead of a boolean new/old like this. This could be refactored to make more sense, I put FIXME comments in mentioning that this isn't used for v2 and should be refactored.
* there's a lot of boilerplate in here (setters/getters, etc) - this patch makes pretty much everything optional, so we could refactor this to be a lot less verbose now
Assignee: laura → rhelmer
Status: NEW → ASSIGNED
Attachment #562577 -
Flags: review?(nrthomas)
Attachment #562577 -
Flags: review?(laura)
Attachment #562577 -
Flags: review?(bhearsum)
Attachment #562577 -
Flags: feedback?(morgamic)
Assignee | ||
Comment 55•13 years ago
|
||
Same as attachment 562577 [details] [diff] [review] plus:
* add acceptance tests (passes in my local VM, jenkins runs them on checkin also)
* unified diff (CVS ftw!)
* fix a few typos that the acceptance tests found
Attachment #562577 -
Attachment is obsolete: true
Attachment #562593 -
Flags: review?(nrthomas)
Attachment #562593 -
Flags: review?(laura)
Attachment #562593 -
Flags: review?(bhearsum)
Attachment #562593 -
Flags: feedback?(morgamic)
Attachment #562577 -
Flags: review?(nrthomas)
Attachment #562577 -
Flags: review?(laura)
Attachment #562577 -
Flags: review?(bhearsum)
Attachment #562577 -
Flags: feedback?(morgamic)
Assignee | ||
Comment 56•13 years ago
|
||
For comparison to attachment 552490 [details], here is an update.xml generated using the patch in attachment 552490 [details] with the synthetic data we use for acceptance tests.
Assignee | ||
Comment 57•13 years ago
|
||
Comment on attachment 562594 [details]
sample update.xml
Rob, does this look reasonable?
Attachment #562594 -
Flags: review?(robert.bugzilla)
Comment 58•13 years ago
|
||
Comment on attachment 562593 [details] [diff] [review]
expose new attributes via AUS
Review of attachment 562593 [details] [diff] [review]:
-----------------------------------------------------------------
Better than v1 :)
One question: did you mean to set the current snippet schema version to 2? In patch.class.php:
define('SNIPPET_SCHEMA_VER_2', 2);
define('CURRENT_SNIPPET_SCHEMA_VER', 1);
If the answer is yes then r+.
Attachment #562593 -
Flags: review?(laura) → review+
Assignee | ||
Comment 59•13 years ago
|
||
(In reply to Laura Thomson :laura from comment #58)
> Comment on attachment 562593 [details] [diff] [review] [diff] [details] [review]
> expose new attributes via AUS
>
> Review of attachment 562593 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
>
> Better than v1 :)
> One question: did you mean to set the current snippet schema version to 2?
> In patch.class.php:
> define('SNIPPET_SCHEMA_VER_2', 2);
> define('CURRENT_SNIPPET_SCHEMA_VER', 1);
> If the answer is yes then r+.
CURRENT_SNIPPET_SCHEMA_VER actually isn't used now (was only used in a single elseif before), and is a confusing name anyway IMO (AUS needs to always detect what version the snippet in question is to be backwards compatible).
Probably best to just remove it and be explicit about the snippet schema version, this is how the URL schema version is handled for instance.
Assignee | ||
Comment 60•13 years ago
|
||
(In reply to Robert Helmer [:rhelmer] from comment #56)
> Created attachment 562594 [details]
> sample update.xml
>
> For comparison to attachment 552490 [details], here is an update.xml
> generated using the patch in attachment 552490 [details] with the synthetic
"generated using the patch in attachment 562593 [details] [diff] [review]"
Assignee | ||
Comment 61•13 years ago
|
||
This is the Verify.txt (acceptance tests) that I intended in attachment 562593 [details] [diff] [review].
Attachment #562653 -
Flags: review?(nrthomas)
Comment 62•13 years ago
|
||
rhelmer, I had a read through and came up with a couple of queries.
1, the test on billboardURL doesn't look quite right. At the moment I think the TRUE value in Verify.txt is just setting the value in Verify.py, and not doing any testing. Perhaps you need to add a hasBillboardURL function there similar to hasCompleteUpdate ? Was also wondering why there is a string check here but hardcoded values for the likes of hasActions, hasOpenURL.
2, it might be risky to allow displayVersion, appVersion, and platformVersion to be optional, if the client app requires them. Can we treat them like the version params for v1, at least at the level of patch or update ? Not sure that actually causes AUS to not serve an update if they're missing but would be worth checking. Its a little scary how trusting AUS is that the snippets will be well formed (so says he doing the snippet generation).
Comment 63•13 years ago
|
||
Comment on attachment 562593 [details] [diff] [review]
expose new attributes via AUS
Review of attachment 562593 [details] [diff] [review]:
-----------------------------------------------------------------
::: xml/inc/patch.class.php
@@ +257,5 @@
> + }
> +
> + // Store information found only in complete snippets.
> + // This information is tied to the <update> element.
> + if ($this->isComplete()) {
This is probably obvious to those that know this code better, but I'm not sure what you mean by "found only in complete snippets" here. Nick's example snippets have all of these attributes in the partials and the completes, and these aren't tied to a specific <update> block in the XML AFAICT. I'm sure I'm missing something here.
Attachment #562593 -
Flags: review?(bhearsum) → review+
Assignee | ||
Comment 64•13 years ago
|
||
(In reply to Nick Thomas [:nthomas] (out sick) from comment #62)
> rhelmer, I had a read through and came up with a couple of queries.
>
> 1, the test on billboardURL doesn't look quite right. At the moment I think
> the TRUE value in Verify.txt is just setting the value in Verify.py, and not
> doing any testing. Perhaps you need to add a hasBillboardURL function there
> similar to hasCompleteUpdate ? Was also wondering why there is a string
> check here but hardcoded values for the likes of hasActions, hasOpenURL.
Right you are, thanks! Will fix in next patch.
> 2, it might be risky to allow displayVersion, appVersion, and
> platformVersion to be optional, if the client app requires them. Can we
> treat them like the version params for v1, at least at the level of patch or
> update ? Not sure that actually causes AUS to not serve an update if they're
> missing but would be worth checking. Its a little scary how trusting AUS is
> that the snippets will be well formed (so says he doing the snippet
> generation).
Yes, I think really it'd be best to have a minimal set of values that are required for each snippet schema version. I shied away from this for the latest patch since I was worried it'd be pretty invasive (and pushed it off to "needs refactoring" comments instead), but I think I see a reasonable way to do it.
I'll attach a new patch shortly.
Assignee | ||
Comment 65•13 years ago
|
||
(In reply to Ben Hearsum [:bhearsum] from comment #63)
> Comment on attachment 562593 [details] [diff] [review] [diff] [details] [review]
> expose new attributes via AUS
>
> Review of attachment 562593 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
>
> ::: xml/inc/patch.class.php
> @@ +257,5 @@
> > + }
> > +
> > + // Store information found only in complete snippets.
> > + // This information is tied to the <update> element.
> > + if ($this->isComplete()) {
>
> This is probably obvious to those that know this code better, but I'm not
> sure what you mean by "found only in complete snippets" here. Nick's example
> snippets have all of these attributes in the partials and the completes, and
> these aren't tied to a specific <update> block in the XML AFAICT. I'm sure
> I'm missing something here.
Hm I actually copied the "snippet schema version 1" section as a starting point for this (could've merged it but I don't want to risk breaking old paths with this patch).
The comment is true in that we only read these attributes from the complete update info, since all updates are required to have a complete update specified (and optionally may have a partial specified). The intent is so that the client can fall back to complete if partial fails to apply.
Assignee | ||
Comment 66•13 years ago
|
||
* pass snippetSchemaVersion as part of Update object, so xml->startUpdate can use it
Not sure if this is what you intended; the version info is still optional, but now there is no way version info from schema1/schema2 could be mixed up. We could return a blank update instead, but these missing should not do harm (the client just may not actually take the update).
It's easy to make this more strict now that this work is done though. The reason I didn't do this in Update or Patch or somewhere else is simply that it require more redundant boiler-plate code, and there's already the confusing "hasUpdateInfo" check. I think the whole thing should be refactored to be a schema check that sets the update up appropriately instead, but that's a big patch for this bug.
* remove CURRENT_SNIPPET_SCHEMA_VER - need to check explicitly against the version (using constants)
* fixed hasBillboardURL test in Verify.txt
Attachment #562593 -
Attachment is obsolete: true
Attachment #562653 -
Attachment is obsolete: true
Attachment #562828 -
Flags: review?(nrthomas)
Attachment #562653 -
Flags: review?(nrthomas)
Attachment #562593 -
Flags: review?(nrthomas)
Attachment #562593 -
Flags: feedback?(morgamic)
Reporter | ||
Comment 67•13 years ago
|
||
Comment on attachment 562594 [details]
sample update.xml
Looks fine though I noticed you don't have an example of showURL in actions.
Attachment #562594 -
Flags: review?(robert.bugzilla) → review+
Comment 68•13 years ago
|
||
Comment on attachment 562828 [details] [diff] [review]
[checked in] expose new attributes via AUS
Looks good. Lets go with this, and assume this code is going away in the near term so that refactoring is probably not worth the effort.
Attachment #562828 -
Flags: review?(nrthomas) → review+
Comment 69•13 years ago
|
||
Comment on attachment 562828 [details] [diff] [review]
[checked in] expose new attributes via AUS
Checking in tests/Verify.py;
/cvsroot/mozilla/webtools/aus/tests/Verify.py,v <-- Verify.py
new revision: 1.10; previous revision: 1.9
done
Checking in tests/Verify.txt;
/cvsroot/mozilla/webtools/aus/tests/Verify.txt,v <-- Verify.txt
new revision: 1.24; previous revision: 1.23
done
RCS file: /cvsroot/mozilla/webtools/aus/tests/data/3/Synthetic/1.0/platform/2000000001/locale/channel/complete.txt,v
done
Checking in tests/data/3/Synthetic/1.0/platform/2000000001/locale/channel/complete.txt;
/cvsroot/mozilla/webtools/aus/tests/data/3/Synthetic/1.0/platform/2000000001/locale/channel/complete.txt,v <-- complete.txt
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/webtools/aus/tests/data/3/Synthetic/1.0/platform/2000000001/locale/channel/partial.txt,v
done
Checking in tests/data/3/Synthetic/1.0/platform/2000000001/locale/channel/partial.txt;
/cvsroot/mozilla/webtools/aus/tests/data/3/Synthetic/1.0/platform/2000000001/locale/channel/partial.txt,v <-- partial.txt
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/webtools/aus/tests/data/3/Synthetic/1.0/platform/2000000002/locale/channel/complete.txt,v
done
Checking in tests/data/3/Synthetic/1.0/platform/2000000002/locale/channel/complete.txt;
/cvsroot/mozilla/webtools/aus/tests/data/3/Synthetic/1.0/platform/2000000002/locale/channel/complete.txt,v <-- complete.txt
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/webtools/aus/tests/data/3/Synthetic/1.0/platform/2000000002/locale/channel/partial.txt,v
done
Checking in tests/data/3/Synthetic/1.0/platform/2000000002/locale/channel/partial.txt;
/cvsroot/mozilla/webtools/aus/tests/data/3/Synthetic/1.0/platform/2000000002/locale/channel/partial.txt,v <-- partial.txt
initial revision: 1.1
done
Checking in xml/index.php;
/cvsroot/mozilla/webtools/aus/xml/index.php,v <-- index.php
new revision: 1.36; previous revision: 1.35
done
Checking in xml/inc/patch.class.php;
/cvsroot/mozilla/webtools/aus/xml/inc/patch.class.php,v <-- patch.class.php
new revision: 1.31; previous revision: 1.30
done
Checking in xml/inc/update.class.php;
/cvsroot/mozilla/webtools/aus/xml/inc/update.class.php,v <-- update.class.php
new revision: 1.3; previous revision: 1.2
done
Checking in xml/inc/xml.class.php;
/cvsroot/mozilla/webtools/aus/xml/inc/xml.class.php,v <-- xml.class.php
new revision: 1.7; previous revision: 1.6
done
Should I be doing any tagging for this landing?
Attachment #562828 -
Attachment description: expose new attributes via AUS → [checked in] expose new attributes via AUS
Assignee | ||
Comment 70•13 years ago
|
||
(In reply to Ben Hearsum [:bhearsum] from comment #69)
Thanks for landing (while I get my CVS access in order)!
> Should I be doing any tagging for this landing?
Yes it's basically still the same as https://wiki.mozilla.org/Webtools:CVS_Guidelines
1) tag as : AUS2_RTM_`date +%Y%M%d%H%M`
2) move ("cvs tag -F") AUS2_STAGING to this tag
Once it looks good on staging we can verify this bug, and then file a push bug (after moving AUS2_PRODUCTION tag to point to the same place as STAGING).
BTW looks like tests pass, and I've double-checked that the new tests got loaded automatically:
https://jenkins.mozilla.org/job/AUSv2/145/
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment 71•13 years ago
|
||
(In reply to Robert Helmer [:rhelmer] from comment #70)
> 1) tag as : AUS2_RTM_`date +%Y%M%d%H%M`
> 2) move ("cvs tag -F") AUS2_STAGING to this tag
Done:
[] bhearsum@voot:~/Mozilla/checkouts/clean/aus$ cvs tag AUS2_RTM_`date +%Y%M%d%H%M`
T README
T sanity/README
T sanity/index.php
T sanity/sanity.inc.php
T sanity/sanity.ini
T sanity/sanity.php
T sanity/control/AUS2_Prod/Fx_1.5b1_Linux.xml
T sanity/control/AUS2_Prod/Fx_1.5b1_Mac.xml
T sanity/control/AUS2_Prod/Fx_1.5b1_Win.xml
T sanity/control/AUS2_Prod/Fx_1.5b2_Linux.xml
T sanity/control/AUS2_Prod/Fx_1.5b2_Mac.xml
T sanity/control/AUS2_Prod/Fx_1.5b2_Win.xml
T sanity/control/AUS2_Prod/Tb_1.5b1_Linux.xml
T sanity/control/AUS2_Prod/Tb_1.5b1_Mac.xml
T sanity/control/AUS2_Prod/Tb_1.5b1_Win.xml
T sanity/control/AUS2_Prod/Tb_1.5b2_Linux.xml
T sanity/control/AUS2_Prod/Tb_1.5b2_Mac.xml
T sanity/control/AUS2_Prod/Tb_1.5b2_Win.xml
T tests/README
T tests/Verify.py
T tests/Verify.txt
T tests/data/2/Synthetic/1.5.0.x/platform/1100000001/locale/complete.txt
T tests/data/2/Synthetic/1.5.0.x/platform/1100000001/locale/partial.txt
T tests/data/2/Synthetic/1.5.0.x/platform/1100000002/locale/complete.txt
T tests/data/2/Synthetic/1.5.0.x/platform/1100000002/locale/partial.txt
T tests/data/2/Synthetic/1.5.0.x/platform/1100000003/locale/complete.txt
T tests/data/2/Synthetic/1.5.0.x/platform/1100000003/locale/partial.txt
T tests/data/2/Synthetic/2.0.0.x/platform/1100000001/locale/complete.txt
T tests/data/2/Synthetic/2.0.0.x/platform/1100000001/locale/partial.txt
T tests/data/2/Synthetic/2.0.0.x/platform/1100000001/otherlocale/complete.txt
T tests/data/2/Synthetic/2.0.0.x/platform/1100000001/otherlocale/partial.txt
T tests/data/2/Synthetic/2.0.0.x/platform/1100000002/locale/complete.txt
T tests/data/2/Synthetic/2.0.0.x/platform/1100000002/locale/partial.txt
T tests/data/2/Synthetic/2.0.0.x/platform/1100000002/otherlocale/complete.txt
T tests/data/2/Synthetic/2.0.0.x/platform/1100000002/otherlocale/partial.txt
T tests/data/2/Synthetic/2.0.0.x/platform/1100000003/locale/complete.txt
T tests/data/2/Synthetic/2.0.0.x/platform/1100000003/locale/partial.txt
T tests/data/2/Synthetic/2.0.0.x-branch/platform/1100000001/locale/complete.txt
T tests/data/2/Synthetic/2.0.0.x-branch/platform/1100000001/locale/partial.txt
T tests/data/2/Synthetic/2.0.0.x-branch/platform/1100000002/locale/complete.txt
T tests/data/2/Synthetic/2.0.0.x-branch/platform/1100000002/locale/partial.txt
T tests/data/2/Synthetic/2.0.0.x-branch/platform/1100000003/locale/complete.txt
T tests/data/2/Synthetic/2.0.0.x-branch/platform/1100000003/locale/partial.txt
T tests/data/3/Synthetic/1.0/platform/1000000001/locale/channel/complete.txt
T tests/data/3/Synthetic/1.0/platform/1000000001/locale/channel/partial.txt
T tests/data/3/Synthetic/1.0/platform/1000000001/locale/channel-cck-partner/complete.txt
T tests/data/3/Synthetic/1.0/platform/1000000001/locale/channel-cck-partner/partial.txt
T tests/data/3/Synthetic/1.0/platform/1000000002/locale/bouncerchannel/complete.txt
T tests/data/3/Synthetic/1.0/platform/1000000002/locale/bouncerchannel/partial.txt
T tests/data/3/Synthetic/1.0/platform/1000000002/locale/channel/complete.txt
T tests/data/3/Synthetic/1.0/platform/1000000002/locale/channel/partial.txt
T tests/data/3/Synthetic/1.0/platform/2000000001/locale/channel/complete.txt
T tests/data/3/Synthetic/1.0/platform/2000000001/locale/channel/partial.txt
T tests/data/3/Synthetic/1.0/platform/2000000002/locale/channel/complete.txt
T tests/data/3/Synthetic/1.0/platform/2000000002/locale/channel/partial.txt
T tests/data/3/Synthetic/1.0/platform/3000000001/locale/channel/complete.txt
T tests/data/3/Synthetic/1.0/platform/3000000001/locale/channel/partial.txt
T tests/data/3/Synthetic/1.0/platform/3000000001/locale/channel-cck-partner/complete.txt
T tests/data/3/Synthetic/1.0/platform/3000000001/locale/channel-cck-partner/partial.txt
T tests/data/3/Synthetic/1.0/platform/4000000001/locale/channel/complete.txt
T tests/data/3/Synthetic/1.0/platform/4000000001/locale/channel/partial.txt
T tests/data/3/Synthetic/1.0/platform/4000000002/locale/channel/complete.txt
T tests/data/3/Synthetic/1.0/platform/4000000002/locale/channel/partial.txt
T tests/data/3/Synthetic/1.0/platform/4000000002/locale/channel-cck-partner/complete.txt
T tests/data/3/Synthetic/1.0/platform/4000000002/locale/channel-cck-partner/partial.txt
T tests/data/3/Synthetic/1.0/platform/5000000001/locale/channel-cck-partner/complete.txt
T tests/data/3/Synthetic/1.0/platform/5000000001/locale/channel-cck-partner/partial.txt
T tests/data/3/Synthetic/1.0/platform/7000000001/locale/channel/complete.txt
T tests/data/3/Synthetic/1.0/platform/7000000001/locale/channel/partial.txt
T tests/data/3/Synthetic/1.0/platform/7000000002/locale/nightly/complete.txt
T tests/data/3/Synthetic/1.0/platform/7000000002/locale/nightly/partial.txt
T tests/data/3/Synthetic/1.0/platform/7000000003/locale/channel/complete.txt
T tests/data/3/Synthetic/1.0/platform/7000000003/locale/channel/partial.txt
T tests/data/3/Synthetic/1.0/platform/7000000003/locale/nightly/complete.txt
T tests/data/3/Synthetic/1.0/platform/7000000003/locale/nightly/partial.txt
T tests/data/3/Synthetic/1.0/platform/8000000001/locale/channel/complete.txt
T tests/data/3/Synthetic/1.0/platform/8000000001/locale/channel/partial.txt
T tests/data/3/Synthetic/1.0/platform/8000000002/locale/channel/complete.txt
T tests/data/3/Synthetic/1.0/platform/8000000002/locale/channel/partial.txt
T tests/data/3/Synthetic/1.0/platform/8000000003/locale/channel/complete.txt
T tests/data/3/Synthetic/1.0/platform/8000000003/locale/channel/partial.txt
T tests/data/3/Synthetic/1.0/platform/8000000004/locale/channel/complete.txt
T tests/data/3/Synthetic/1.0/platform/8000000004/locale/channel/partial.txt
T tests/data/3/Synthetic/1.0/platform/9000000001/locale/channel/complete.txt
T tests/data/3/Synthetic/1.0/platform/9000000001/locale/channel/partial.txt
T xml/README
T xml/flush.php
T xml/htaccess.dist
T xml/index.php
T xml/status.php
T xml/bug602275/please_reinstall.xml
T xml/inc/aus.class.php
T xml/inc/config-dist.php
T xml/inc/config-test.php
T xml/inc/init.php
T xml/inc/memcaching.php
T xml/inc/patch.class.php
T xml/inc/update.class.php
T xml/inc/xml.class.php
[] bhearsum@voot:~/Mozilla/checkouts/clean/aus$ cvs tag AUS2_STAGING
W tests/Verify.py : AUS2_STAGING already exists on version 1.9 : NOT MOVING tag to version 1.10
W tests/Verify.txt : AUS2_STAGING already exists on version 1.20 : NOT MOVING tag to version 1.24
T tests/data/3/Synthetic/1.0/platform/2000000001/locale/channel/complete.txt
T tests/data/3/Synthetic/1.0/platform/2000000001/locale/channel/partial.txt
T tests/data/3/Synthetic/1.0/platform/2000000002/locale/channel/complete.txt
T tests/data/3/Synthetic/1.0/platform/2000000002/locale/channel/partial.txt
W tests/data/3/Synthetic/1.0/platform/9000000001/locale/channel/complete.txt : AUS2_STAGING already exists on version 1.2 : NOT MOVING tag to version 1.5
W tests/data/3/Synthetic/1.0/platform/9000000001/locale/channel/partial.txt : AUS2_STAGING already exists on version 1.2 : NOT MOVING tag to version 1.5
W xml/index.php : AUS2_STAGING already exists on version 1.32 : NOT MOVING tag to version 1.36
W xml/inc/config-dist.php : AUS2_STAGING already exists on version 1.135 : NOT MOVING tag to version 1.163
W xml/inc/config-test.php : AUS2_STAGING already exists on version 1.4 : NOT MOVING tag to version 1.7
W xml/inc/patch.class.php : AUS2_STAGING already exists on version 1.27 : NOT MOVING tag to version 1.31
W xml/inc/update.class.php : AUS2_STAGING already exists on version 1.2 : NOT MOVING tag to version 1.3
W xml/inc/xml.class.php : AUS2_STAGING already exists on version 1.6 : NOT MOVING tag to version 1.7
[] bhearsum@voot:~/Mozilla/checkouts/clean/aus$ cvs tag -F AUS2_STAGING
T tests/Verify.py
T tests/Verify.txt
T tests/data/3/Synthetic/1.0/platform/9000000001/locale/channel/complete.txt
T tests/data/3/Synthetic/1.0/platform/9000000001/locale/channel/partial.txt
T xml/index.php
T xml/inc/config-dist.php
T xml/inc/config-test.php
T xml/inc/patch.class.php
T xml/inc/update.class.php
T xml/inc/xml.class.php
Comment 72•13 years ago
|
||
Whoo! This is on production, right? I assume by the RTM tag but just want to make sure.
Assignee | ||
Comment 73•13 years ago
|
||
(In reply to Christian Legnitto [:LegNeato] from comment #72)
> Whoo! This is on production, right? I assume by the RTM tag but just want to
> make sure.
It hasn't been pushed yet, but it's ready to go.
It is live on staging, I'd like to get some testing on it if possible.
Pretty sure we need bug 682805 to land, and snippets to be generated w/ attributes, to test properly.
Comment 74•13 years ago
|
||
Ok, good to know...thought it might be the other way around :-)
Comment 75•13 years ago
|
||
I ran a comparison of the current production code vs the new code tagged for staging, using the 'old' style of snippets we currently have live. It queried on the release channel for Firefox 3.0.19, 3.6.22 and 6.0.1 (all platforms and all locales).
There were no differences in the response of the two code bases, so think we can push the new code to production.
Comment 76•13 years ago
|
||
rhelmer, I tried to set up aus2-staging to specifically test that we can suppress the What's New page (ie actions=silent), but that's difficult because our releases from 7.0b<something> through to 8.0b3 already do that by setting a Firefox pref (bug 685727, bug 689679). We don't have nightly builds on mozilla-beta, and the dep builds don't produce mar files.
Was able to verify that setting displayVersion, appVersion, platformVersion, openURL and actions=showURL worked (without looking at add-on compat).
What sort of testing did you have in mind at comment #73 ?
Assignee | ||
Comment 77•13 years ago
|
||
(In reply to Nick Thomas [:nthomas] from comment #76)
> rhelmer, I tried to set up aus2-staging to specifically test that we can
> suppress the What's New page (ie actions=silent), but that's difficult
> because our releases from 7.0b<something> through to 8.0b3 already do that
> by setting a Firefox pref (bug 685727, bug 689679). We don't have nightly
> builds on mozilla-beta, and the dep builds don't produce mar files.
>
> Was able to verify that setting displayVersion, appVersion, platformVersion,
> openURL and actions=showURL worked (without looking at add-on compat).
>
> What sort of testing did you have in mind at comment #73 ?
I think that's probably fine, I just wanted to make sure someone had looked the output on stage to make sure it looks right.
This does pass all automated tests so I think we're good to go with the tests you've done.
Status: RESOLVED → VERIFIED
Assignee | ||
Comment 78•13 years ago
|
||
BTW nthomas is going to file the IT bug to get this pushed out, needs to be coordinated with releases etc.
Comment 79•13 years ago
|
||
Tagging for production (aus/xml only):
$ cvs up -r AUS2_RTM_201113030913
# make sure we don't zap the change from bug 694077 that landed later
$ cvs up -r AUS2_PRODUCTION inc/config-dist.php
$ cvs tag AUS2_PRODUCTION
W index.php : AUS2_PRODUCTION already exists on version 1.35 : NOT MOVING tag to version 1.36
W inc/patch.class.php : AUS2_PRODUCTION already exists on version 1.30 : NOT MOVING tag to version 1.31
W inc/update.class.php : AUS2_PRODUCTION already exists on version 1.2 : NOT MOVING tag to version 1.3
W inc/xml.class.php : AUS2_PRODUCTION already exists on version 1.6 : NOT MOVING tag to version 1.7
$ cvs tag -F AUS2_PRODUCTION
T index.php
T inc/patch.class.php
T inc/update.class.php
T inc/xml.class.php
Bug 696278 for the push live by IT.
Depends on: 696278
You need to log in
before you can comment on or make changes to this bug.
Description
•