Issue 2796 - Moved logic to extract Safari Developer ID to buildtools

Issue 2796 - Moved logic to extract Safari Developer ID to buildtools

[kzar, 2015-07-16 15:14:06]

https://codereview.adblockplus.org/29322523/diff/29322524/sitescripts/extensi...
File sitescripts/extensions/bin/updateUpdateManifests.py (right):

https://codereview.adblockplus.org/29322523/diff/29322524/sitescripts/extensi...
sitescripts/extensions/bin/updateUpdateManifests.py:58: 'certificateID':
get_developer_identifier(get_certificates_and_key(repo.keyFile)[0]),
Won't this be a list of certificate IDs instead of just one?

[Sebastian Noack, 2015-07-16 15:15:13]

On 2015/07/16 15:14:06, kzar wrote:
>
https://codereview.adblockplus.org/29322523/diff/29322524/sitescripts/extensi...
> File sitescripts/extensions/bin/updateUpdateManifests.py (right):
> 
>
https://codereview.adblockplus.org/29322523/diff/29322524/sitescripts/extensi...
> sitescripts/extensions/bin/updateUpdateManifests.py:58: 'certificateID':
> get_developer_identifier(get_certificates_and_key(repo.keyFile)[0]),
> Won't this be a list of certificate IDs instead of just one?

get_certificate_and_key() returns a tuple with a list of certificates as first
item and the private key as second item.

[kzar, 2015-07-16 15:22:15]

On 2015/07/16 15:15:13, Sebastian Noack wrote:
> On 2015/07/16 15:14:06, kzar wrote:
> >
>
https://codereview.adblockplus.org/29322523/diff/29322524/sitescripts/extensi...
> > File sitescripts/extensions/bin/updateUpdateManifests.py (right):
> > 
> >
>
https://codereview.adblockplus.org/29322523/diff/29322524/sitescripts/extensi...
> > sitescripts/extensions/bin/updateUpdateManifests.py:58: 'certificateID':
> > get_developer_identifier(get_certificates_and_key(repo.keyFile)[0]),
> > Won't this be a list of certificate IDs instead of just one?
> 
> get_certificate_and_key() returns a tuple with a list of certificates as first
> item and the private key as second item.

Right, so from the key name at least it sounded like one certificate ID is
required instead of a list of certificate IDs?

[Sebastian Noack, 2015-07-16 15:25:40]

On 2015/07/16 15:22:15, kzar wrote:
> On 2015/07/16 15:15:13, Sebastian Noack wrote:
> > On 2015/07/16 15:14:06, kzar wrote:
> > >
> >
>
https://codereview.adblockplus.org/29322523/diff/29322524/sitescripts/extensi...
> > > File sitescripts/extensions/bin/updateUpdateManifests.py (right):
> > > 
> > >
> >
>
https://codereview.adblockplus.org/29322523/diff/29322524/sitescripts/extensi...
> > > sitescripts/extensions/bin/updateUpdateManifests.py:58: 'certificateID':
> > > get_developer_identifier(get_certificates_and_key(repo.keyFile)[0]),
> > > Won't this be a list of certificate IDs instead of just one?
> > 
> > get_certificate_and_key() returns a tuple with a list of certificates as
first
> > item and the private key as second item.
> 
> Right, so from the key name at least it sounded like one certificate ID is
> required instead of a list of certificate IDs?

Well, the right one is needed. Therefore we iterate over all certificates in
get_develover_id() until we find the developer ID in any certificate.

[kzar, 2015-07-16 15:32:29]

On 2015/07/16 15:25:40, Sebastian Noack wrote:
> On 2015/07/16 15:22:15, kzar wrote:
> > On 2015/07/16 15:15:13, Sebastian Noack wrote:
> > > On 2015/07/16 15:14:06, kzar wrote:
> > > >
> > >
> >
>
https://codereview.adblockplus.org/29322523/diff/29322524/sitescripts/extensi...
> > > > File sitescripts/extensions/bin/updateUpdateManifests.py (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.adblockplus.org/29322523/diff/29322524/sitescripts/extensi...
> > > > sitescripts/extensions/bin/updateUpdateManifests.py:58: 'certificateID':
> > > > get_developer_identifier(get_certificates_and_key(repo.keyFile)[0]),
> > > > Won't this be a list of certificate IDs instead of just one?
> > > 
> > > get_certificate_and_key() returns a tuple with a list of certificates as
> first
> > > item and the private key as second item.
> > 
> > Right, so from the key name at least it sounded like one certificate ID is
> > required instead of a list of certificate IDs?
> 
> Well, the right one is needed. Therefore we iterate over all certificates in
> get_develover_id() until we find the developer ID in any certificate.

Whoops, I see what you mean.

I can't see anything obvious so LGTM but I think Wladimir should double check
this one.

[Sebastian Noack, 2015-07-16 15:47:28]

Sorry, I forgot to remove one of the old imports. Here is a new patch set.

[Wladimir Palant, 2015-07-16 20:47:08]

https://codereview.adblockplus.org/29322523/diff/29322538/sitescripts/extensi...
File sitescripts/extensions/bin/createNightlies.py (right):

https://codereview.adblockplus.org/29322523/diff/29322538/sitescripts/extensi...
sitescripts/extensions/bin/createNightlies.py:34: from
sitescripts.extensions.utils import compareVersions, Configuration,
writeAndroidUpdateManifest
This line is still too long - please break it into two as it was before.

https://codereview.adblockplus.org/29322523/diff/29322538/sitescripts/extensi...
File sitescripts/extensions/bin/updateUpdateManifests.py (right):

https://codereview.adblockplus.org/29322523/diff/29322538/sitescripts/extensi...
sitescripts/extensions/bin/updateUpdateManifests.py:31: from
buildtools.packagerSafari import get_developer_identifier,
get_certificates_and_key
Nit: This line is too long.

https://codereview.adblockplus.org/29322523/diff/29322538/sitescripts/extensi...
sitescripts/extensions/bin/updateUpdateManifests.py:58: 'certificateID':
get_developer_identifier(get_certificates_and_key(repo.keyFile)[0]),
Nit: This line is too long.

[Sebastian Noack, 2015-07-17 11:10:59]

https://codereview.adblockplus.org/29322523/diff/29322538/sitescripts/extensi...
File sitescripts/extensions/bin/createNightlies.py (right):

https://codereview.adblockplus.org/29322523/diff/29322538/sitescripts/extensi...
sitescripts/extensions/bin/createNightlies.py:34: from
sitescripts.extensions.utils import compareVersions, Configuration,
writeAndroidUpdateManifest
On 2015/07/16 20:47:08, Wladimir Palant wrote:
> This line is still too long - please break it into two as it was before.

Done.

https://codereview.adblockplus.org/29322523/diff/29322538/sitescripts/extensi...
File sitescripts/extensions/bin/updateUpdateManifests.py (right):

https://codereview.adblockplus.org/29322523/diff/29322538/sitescripts/extensi...
sitescripts/extensions/bin/updateUpdateManifests.py:31: from
buildtools.packagerSafari import get_developer_identifier,
get_certificates_and_key
On 2015/07/16 20:47:08, Wladimir Palant wrote:
> Nit: This line is too long.

Done.

https://codereview.adblockplus.org/29322523/diff/29322538/sitescripts/extensi...
sitescripts/extensions/bin/updateUpdateManifests.py:58: 'certificateID':
get_developer_identifier(get_certificates_and_key(repo.keyFile)[0]),
On 2015/07/16 20:47:08, Wladimir Palant wrote:
> Nit: This line is too long.

Done.

[Wladimir Palant, 2015-07-17 11:39:51]

LGTM

[Sebastian Noack, 2015-07-17 11:57:36]

Added the required dependency update.

For reference, currently the buildtools dependency is only used in the
development environment, while the server always uses the latest version of
buildtools.

[Wladimir Palant, 2015-07-17 11:58:28]

Still LGTM.

[kzar, 2015-07-17 14:20:47]

Still LGTM