Issue 29374642: Issue 4865 - Make sure Identity.Name attribute is different for devbuilds for Edge (Closed)
DescriptionIssue 4865 - Make sure Identity.Name attribute is different for devbuilds for Edge
Patch Set 1 #
Total comments: 3
Patch Set 2 : Use separate entries for app_id in release and devbuild builds. Update the tests. #
Total comments: 4
Patch Set 3 : Rename a variable. Remove unnecessary conditional. #
MessagesTotal messages: 11
LGTM
https://codereview.adblockplus.org/29374642/diff/29374643/packagerEdge.py File packagerEdge.py (right): https://codereview.adblockplus.org/29374642/diff/29374643/packagerEdge.py#new... packagerEdge.py:96: params['app_id'] += 'developmentbuild' During the review of the initial implementation we discussed whether we should automatically generate the app_id, as it is essentially '{}.{}'.format(author, name).replace(' ', ''), and we already have the "author" and "name" configured separately. In the end Vasily convinced me that configuring the app_id in the metadata file is preferable over code that derives the app_id automatically. Later, when it turned out that we need a different app_extension_id (not to be confused with app_id) for the Adblock Plus release and development builds, hard-coding that logic in the packager like this patch does, got rejected in favor of having separate options in the metadata file for the release and development builds. Both decisions, and the resulting implementation, is inconsistent with the change suggested here. Instead, we should either properly derive the app_id from the extensions's author and name, or replace app_id with app_id_release and app_id_devbuild (as we did for app_extension_id_*). Vasily, Winsley; since you worked on this before, what do you think?
https://codereview.adblockplus.org/29374642/diff/29374643/packagerEdge.py File packagerEdge.py (right): https://codereview.adblockplus.org/29374642/diff/29374643/packagerEdge.py#new... packagerEdge.py:96: params['app_id'] += 'developmentbuild' On 2017/02/07 14:44:38, Sebastian Noack wrote: > During the review of the initial implementation we discussed whether we should > automatically generate the app_id, as it is essentially '{}.{}'.format(author, > name).replace(' ', ''), and we already have the "author" and "name" configured > separately. In the end Vasily convinced me that configuring the app_id in the > metadata file is preferable over code that derives the app_id automatically. > > Later, when it turned out that we need a different app_extension_id (not to be > confused with app_id) for the Adblock Plus release and development builds, > hard-coding that logic in the packager like this patch does, got rejected in > favor of having separate options in the metadata file for the release and > development builds. > > Both decisions, and the resulting implementation, is inconsistent with the > change suggested here. Instead, we should either properly derive the app_id from > the extensions's author and name, or replace app_id with app_id_release and > app_id_devbuild (as we did for app_extension_id_*). > > Vasily, Winsley; since you worked on this before, what do you think? I prefer the second option. Constructing ids from other variables seems to give a wrong idea of what it is. Since app_id links the package to its entry in the Windows store (unless I'm misremembering the context) I think it's more logical to set it manually. That was also the reason why I preferred that approach when this code was initially written.
On 2017/02/07 14:57:41, Vasily Kuznetsov wrote: > https://codereview.adblockplus.org/29374642/diff/29374643/packagerEdge.py > File packagerEdge.py (right): > > https://codereview.adblockplus.org/29374642/diff/29374643/packagerEdge.py#new... > packagerEdge.py:96: params['app_id'] += 'developmentbuild' > On 2017/02/07 14:44:38, Sebastian Noack wrote: > > During the review of the initial implementation we discussed whether we should > > automatically generate the app_id, as it is essentially '{}.{}'.format(author, > > name).replace(' ', ''), and we already have the "author" and "name" configured > > separately. In the end Vasily convinced me that configuring the app_id in the > > metadata file is preferable over code that derives the app_id automatically. > > > > Later, when it turned out that we need a different app_extension_id (not to be > > confused with app_id) for the Adblock Plus release and development builds, > > hard-coding that logic in the packager like this patch does, got rejected in > > favor of having separate options in the metadata file for the release and > > development builds. > > > > Both decisions, and the resulting implementation, is inconsistent with the > > change suggested here. Instead, we should either properly derive the app_id > from > > the extensions's author and name, or replace app_id with app_id_release and > > app_id_devbuild (as we did for app_extension_id_*). > > > > Vasily, Winsley; since you worked on this before, what do you think? > > I prefer the second option. Constructing ids from other variables seems to give > a wrong idea of what it is. Since app_id links the package to its entry in the > Windows store (unless I'm misremembering the context) I think it's more logical > to set it manually. That was also the reason why I preferred that approach when > this code was initially written. I agree, also you might want the check if the test for the manifest still pass, IIRC they also test the app_id.
https://codereview.adblockplus.org/29374642/diff/29374643/packagerEdge.py File packagerEdge.py (right): https://codereview.adblockplus.org/29374642/diff/29374643/packagerEdge.py#new... packagerEdge.py:96: params['app_id'] += 'developmentbuild' On 2017/02/07 14:57:41, Vasily Kuznetsov wrote: > On 2017/02/07 14:44:38, Sebastian Noack wrote: > > During the review of the initial implementation we discussed whether we should > > automatically generate the app_id, as it is essentially '{}.{}'.format(author, > > name).replace(' ', ''), and we already have the "author" and "name" configured > > separately. In the end Vasily convinced me that configuring the app_id in the > > metadata file is preferable over code that derives the app_id automatically. > > > > Later, when it turned out that we need a different app_extension_id (not to be > > confused with app_id) for the Adblock Plus release and development builds, > > hard-coding that logic in the packager like this patch does, got rejected in > > favor of having separate options in the metadata file for the release and > > development builds. > > > > Both decisions, and the resulting implementation, is inconsistent with the > > change suggested here. Instead, we should either properly derive the app_id > from > > the extensions's author and name, or replace app_id with app_id_release and > > app_id_devbuild (as we did for app_extension_id_*). > > > > Vasily, Winsley; since you worked on this before, what do you think? > > I prefer the second option. Constructing ids from other variables seems to give > a wrong idea of what it is. Since app_id links the package to its entry in the > Windows store (unless I'm misremembering the context) I think it's more logical > to set it manually. That was also the reason why I preferred that approach when > this code was initially written. Done.
https://codereview.adblockplus.org/29374642/diff/29374724/packagerEdge.py File packagerEdge.py (left): https://codereview.adblockplus.org/29374642/diff/29374724/packagerEdge.py#old... packagerEdge.py:88: metadata_id_suffix = 'release' if release_build else 'devbuild' Nit: Perhaps, call this variable just "metadata_suffix" now, that both options that use this mechanism are some kind of ID is rather coincidental. https://codereview.adblockplus.org/29374642/diff/29374724/packagerEdge.py File packagerEdge.py (right): https://codereview.adblockplus.org/29374642/diff/29374724/packagerEdge.py#new... packagerEdge.py:97: raise KeyError(app_id + ' key not found') Do we even need to specifically handle that case? Won't we get a similar error anyway, when calling metadata.get('general', app_id) without prior check?
https://codereview.adblockplus.org/29374642/diff/29374724/packagerEdge.py File packagerEdge.py (left): https://codereview.adblockplus.org/29374642/diff/29374724/packagerEdge.py#old... packagerEdge.py:88: metadata_id_suffix = 'release' if release_build else 'devbuild' On 2017/02/08 07:17:16, Sebastian Noack wrote: > Nit: Perhaps, call this variable just "metadata_suffix" now, that both options > that use this mechanism are some kind of ID is rather coincidental. Done. https://codereview.adblockplus.org/29374642/diff/29374724/packagerEdge.py File packagerEdge.py (right): https://codereview.adblockplus.org/29374642/diff/29374724/packagerEdge.py#new... packagerEdge.py:97: raise KeyError(app_id + ' key not found') On 2017/02/08 07:17:16, Sebastian Noack wrote: > Do we even need to specifically handle that case? Won't we get a similar error > anyway, when calling metadata.get('general', app_id) without prior check? Done.
LGTM
LGTM
LGTM from me too, in case still needed :) 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
