cipd: registerPackage method implementation.

cipd: registerPackage method implementation.

Also some initial data model for package repository.

TBR=nodir@chromium.org,iannucci@chromium.org
BUG=

Committed: https://chromium.googlesource.com/infra/infra/+/9ea72bb7ae1e895bf5832b26414dd0d4eb9c05bf

[Vadim Sh., 2014-12-30 02:22:26]

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
File appengine/chrome_infra_packages/cipd/api.py (right):

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/api.py:194: upload_url, upload_session_id =
service.create_upload_session(
It makes 'cas' service and 'repo' service a bit tangled :( The problem is, CAS
service doesn't know anything about authorization (e.g. who is allowed to
upload) since ACL is formulated in terms of package names (e.g. package
"infra/*" are allowed to be modified by chrome-infra-team group) that CAS
service has no idea about.

There are two ways to make it work:
1) Hide generic 'initiateUpload' CAS call and expose a package repository
specific one (with proper authorization). That is what I do in this CL.
2) Make generic initiateUpload call accept some sort of nonce that grants an
upload of specific item, and make registerPackage check ACL and generate that
nonce. It makes services more decoupled, but introduces a round trip in the
protocol and makes client more knowledgeable.  

¯\_(ツ)_/¯

[nodir, 2014-12-30 22:54:02]

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
File appengine/chrome_infra_packages/cipd/__init__.py (right):

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/__init__.py:6: from .impl import
get_repo_service
You should probably import PackageInstanceSignature as well, since you can't use
RepoService.register_package without it

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
File appengine/chrome_infra_packages/cipd/api.py (right):

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/api.py:98: 1. Attempt to register a package
instance by callling registerPackage(msg).
typo: calllling

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/api.py:115: class Status(messages.Enum):
Blank line after """

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/api.py:127:
'RegisterPackageResponse.Status', 1, required=True)
Why not EnumField(Status, ...) ?

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/api.py:159: raise
endpoints.BadRequestException('Invalid instance ID')
Consider returning different error reasons. A program would have hard time
distinguishing different error responses

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/api.py:160: 
metadata is not validated? To me, either it should be arbitrary key-value pairs,
or it is semantic and validated

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/api.py:179: ent.added_ts = utils.utcnow()
call utcnow once per request

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/api.py:186: request.package_name,
request.instance_id, signatures)
I think you should not add signatures

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
File appengine/chrome_infra_packages/cipd/impl.py (right):

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/impl.py:20: them, but clients do when they
download the package.
Will metadata be signed?

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/impl.py:48: DIGEST_ALGO = 'SHA1'
I assume this will never change since you use shas to identify instances

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/impl.py:57: """True if string looks like a
valida package instance ID."""
typo: valid

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/impl.py:91: """Makes new PackageInstance or
updates signatures on existing one.
"of an existing one"

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/impl.py:95: signatures (passed via
|signatures|) will be added to the package signature
just "...all new |signatures| will ..."

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/impl.py:96: list. Metadata of existing
packages is never changed.
Why you don't just raise an exception of a package is already registered? Just
adding signatures, but not updating metadata does not make sense to me. You
already have add_signatures method

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/impl.py:100: instance_id: identified of the
package instance (SHA1 of package content).
typo?: identity of the package instance

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/impl.py:124: """Updates signatures on
existing package instance.
of an existing package

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/impl.py:128: instance_id: identified of the
package instance (SHA1 of package content).
typo? identity

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/impl.py:135: ValueError is package instance
is missing.
s/is/if

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/impl.py:139: raise ValueError('Package
instance is not registered')
Consider being more explicit, PackageNotFoundError

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/impl.py:144: def is_data_uploaded(self,
package_name, instance_id):
what is data? Maybe rename to "is_instance_uploaded"?

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/impl.py:159: """Opens new session for data
upload to CAS.
typo: Opens a new

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/impl.py:163: instance_id: identified of the
package instance (SHA1 of package content).
typo? identity

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/impl.py:185: date =
ndb.StringProperty(indexed=False)
Why string?

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/impl.py:220: """String that identifies how
the signature was obtained."""
I'd call this property "fingerprint" or something. It does not actually identify
a PackageInstanceSignature because it does not contain the signature.
_add_signature.add_one() has to check signature also.

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/impl.py:244: signature_ids =
ndb.StringProperty(repeated=True)
I'd call this signature_fingerprints. I don't know how you are going to use this
though

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/impl.py:247: """Adds new signatures to
signature list (skips existing ones)."""
document Returns

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/impl.py:260: 
Having a _pre_put_hook in models that checks invariants would be nice, up to you

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/impl.py:270: assert
is_valid_instance_id(instance_id), instance_id
Also validate package name

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
File appengine/chrome_infra_packages/cipd/test/api_test.py (right):

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/test/api_test.py:109: self.mock(impl,
'get_repo_service', mocked_get_repo_service)
lambda: self.repo_service
would fit

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
File appengine/chrome_infra_packages/cipd/test/impl_test.py (right):

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/test/impl_test.py:165: with
self.assertRaises(ValueError):
ValueError is ambiguous. It could be anything, really.

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/test/impl_test.py:176: expected = lambda
key: {
I always thought multi-line lambdas do not satisfy our styleguide, and make
myself defining normals funcs

[Vadim Sh., 2014-12-31 01:27:36]

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
File appengine/chrome_infra_packages/cipd/__init__.py (right):

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/__init__.py:6: from .impl import
get_repo_service
On 2014/12/30 22:54:00, nodir wrote:
> You should probably import PackageInstanceSignature as well, since you can't
use
> RepoService.register_package without it

Removed get_repo_service instead. It is not used outside of the package yet.

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
File appengine/chrome_infra_packages/cipd/api.py (right):

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/api.py:98: 1. Attempt to register a package
instance by callling registerPackage(msg).
On 2014/12/30 22:54:00, nodir wrote:
> typo: calllling

Done.

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/api.py:115: class Status(messages.Enum):
On 2014/12/30 22:54:00, nodir wrote:
> Blank line after """

Done.

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/api.py:127:
'RegisterPackageResponse.Status', 1, required=True)
On 2014/12/30 22:54:00, nodir wrote:
> Why not EnumField(Status, ...) ?

Done. For some reason I though it does like local classes.

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/api.py:159: raise
endpoints.BadRequestException('Invalid instance ID')
On 2014/12/30 22:54:00, nodir wrote:
> Consider returning different error reasons. A program would have hard time
> distinguishing different error responses

Client will do client side validation. Checks here are mostly "asserts".

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/api.py:160: 
On 2014/12/30 22:54:00, nodir wrote:
> metadata is not validated? To me, either it should be arbitrary key-value
pairs,
> or it is semantic and validated

I was planing to add very few required FYI only fields. Custom key-value paris
will be implemented as part of labels system.

In fact, f*ck it, I'll remove notion of "metadata" for now and implement it
later together with labels.

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/api.py:179: ent.added_ts = utils.utcnow()
On 2014/12/30 22:54:00, nodir wrote:
> call utcnow once per request

Done.

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/api.py:186: request.package_name,
request.instance_id, signatures)
On 2014/12/30 22:54:00, nodir wrote:
> I think you should not add signatures

Then there's no way to resign a package. I mean it can be done as a separate API
call probably, but it would complicate the client.

I'd like to keep it as is for now and maybe later implement a separate call,
when client implementation is more clear.

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
File appengine/chrome_infra_packages/cipd/impl.py (right):

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/impl.py:20: them, but clients do when they
download the package.
On 2014/12/30 22:54:01, nodir wrote:
> Will metadata be signed?
There's a separate thing called "manifest" that is inside the package and it is
signed with the package.

Removed the metadata for now, needs more thought.

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/impl.py:48: DIGEST_ALGO = 'SHA1'
On 2014/12/30 22:54:01, nodir wrote:
> I assume this will never change since you use shas to identify instances

Yes.

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/impl.py:57: """True if string looks like a
valida package instance ID."""
On 2014/12/30 22:54:01, nodir wrote:
> typo: valid

Done.

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/impl.py:91: """Makes new PackageInstance or
updates signatures on existing one.
On 2014/12/30 22:54:01, nodir wrote:
> "of an existing one"

Done.

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/impl.py:95: signatures (passed via
|signatures|) will be added to the package signature
On 2014/12/30 22:54:01, nodir wrote:
> just "...all new |signatures| will ..."

Done.

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/impl.py:96: list. Metadata of existing
packages is never changed.
On 2014/12/30 22:54:01, nodir wrote:
> Why you don't just raise an exception of a package is already registered? Just
> adding signatures, but not updating metadata does not make sense to me. You
> already have add_signatures method

Well.. okay. It is more transactions in case of a race condition, but such race
condition is very unlikely.

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/impl.py:100: instance_id: identified of the
package instance (SHA1 of package content).
On 2014/12/30 22:54:01, nodir wrote:
> typo?: identity of the package instance

Done.

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/impl.py:124: """Updates signatures on
existing package instance.
On 2014/12/30 22:54:00, nodir wrote:
> of an existing package

Done.

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/impl.py:128: instance_id: identified of the
package instance (SHA1 of package content).
On 2014/12/30 22:54:00, nodir wrote:
> typo? identity

Copy-pasted typo.

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/impl.py:135: ValueError is package instance
is missing.
On 2014/12/30 22:54:01, nodir wrote:
> s/is/if

Done.

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/impl.py:139: raise ValueError('Package
instance is not registered')
On 2014/12/30 22:54:01, nodir wrote:
> Consider being more explicit, PackageNotFoundError

Done.

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/impl.py:144: def is_data_uploaded(self,
package_name, instance_id):
On 2014/12/30 22:54:01, nodir wrote:
> what is data? Maybe rename to "is_instance_uploaded"?

Done.

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/impl.py:159: """Opens new session for data
upload to CAS.
On 2014/12/30 22:54:01, nodir wrote:
> typo: Opens a new 

Done.

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/impl.py:163: instance_id: identified of the
package instance (SHA1 of package content).
On 2014/12/30 22:54:01, nodir wrote:
> typo? identity

Done.

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/impl.py:185: date =
ndb.StringProperty(indexed=False)
On 2014/12/30 22:54:01, nodir wrote:
> Why string?

Eh.. because I'm lazy. No longer relevant.

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/impl.py:220: """String that identifies how
the signature was obtained."""
On 2014/12/30 22:54:01, nodir wrote:
> I'd call this property "fingerprint" or something. It does not actually
identify
> a PackageInstanceSignature because it does not contain the signature.
> _add_signature.add_one() has to check signature also.

signature_key itself is fingerprint (hash of public key), do not want to reuse
this word.

It identifies how signature was obtained (algorithms and public key used). The
idea is to be able to query for a list of packages signed by a specific key. 

Actually, hash_algo and signature_algo can probably be omitted, so I'll just use
signature_key.

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/impl.py:244: signature_ids =
ndb.StringProperty(repeated=True)
On 2014/12/30 22:54:01, nodir wrote:
> I'd call this signature_fingerprints. I don't know how you are going to use
this
> though

Renamed to signature_keys. I plan to use it for query(signature_ids == <some
id>) to fetch a list of packages signed by a specific key. Maybe later, in the
future...

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/impl.py:247: """Adds new signatures to
signature list (skips existing ones)."""
On 2014/12/30 22:54:01, nodir wrote:
> document Returns

Done.

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/impl.py:260: 
On 2014/12/30 22:54:01, nodir wrote:
> Having a _pre_put_hook in models that checks invariants would be nice, up to
you

Done.

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/impl.py:270: assert
is_valid_instance_id(instance_id), instance_id
On 2014/12/30 22:54:00, nodir wrote:
> Also validate package name

It is validated in package_key

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
File appengine/chrome_infra_packages/cipd/test/api_test.py (right):

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/test/api_test.py:109: self.mock(impl,
'get_repo_service', mocked_get_repo_service)
On 2014/12/30 22:54:01, nodir wrote:
> lambda: self.repo_service
> would fit

Done.

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
File appengine/chrome_infra_packages/cipd/test/impl_test.py (right):

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/test/impl_test.py:165: with
self.assertRaises(ValueError):
On 2014/12/30 22:54:01, nodir wrote:
> ValueError is ambiguous. It could be anything, really.

Done.

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/test/impl_test.py:176: expected = lambda
key: {
On 2014/12/30 22:54:01, nodir wrote:
> I always thought multi-line lambdas do not satisfy our styleguide, and make
> myself defining normals funcs

Never heard of that :)

Generally I avoid line wrapping in lambdas, but if it syntactically correct and
shorter - why not?

[nodir, 2015-01-02 19:13:04]

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
File appengine/chrome_infra_packages/cipd/__init__.py (right):

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/__init__.py:6: from .impl import
get_repo_service
On 2014/12/31 01:27:35, Vadim Sh. wrote:
> On 2014/12/30 22:54:00, nodir wrote:
> > You should probably import PackageInstanceSignature as well, since you can't
> use
> > RepoService.register_package without it
> 
> Removed get_repo_service instead. It is not used outside of the package yet.

Acknowledged.

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
File appengine/chrome_infra_packages/cipd/api.py (right):

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/api.py:160: 
On 2014/12/31 01:27:35, Vadim Sh. wrote:
> On 2014/12/30 22:54:00, nodir wrote:
> > metadata is not validated? To me, either it should be arbitrary key-value
> pairs,
> > or it is semantic and validated
> 
> I was planing to add very few required FYI only fields. Custom key-value paris
> will be implemented as part of labels system.
> 
> In fact, f*ck it, I'll remove notion of "metadata" for now and implement it
> later together with labels.

Acknowledged.

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
File appengine/chrome_infra_packages/cipd/impl.py (right):

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/impl.py:20: them, but clients do when they
download the package.
On 2014/12/31 01:27:35, Vadim Sh. wrote:
> On 2014/12/30 22:54:01, nodir wrote:
> > Will metadata be signed?
> There's a separate thing called "manifest" that is inside the package and it
is
> signed with the package.
> 
> Removed the metadata for now, needs more thought.

Acknowledged.

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/impl.py:270: assert
is_valid_instance_id(instance_id), instance_id
On 2014/12/31 01:27:35, Vadim Sh. wrote:
> On 2014/12/30 22:54:00, nodir wrote:
> > Also validate package name
> 
> It is validated in package_key

Acknowledged.

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
File appengine/chrome_infra_packages/cipd/test/impl_test.py (right):

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/test/impl_test.py:176: expected = lambda
key: {
On 2014/12/31 01:27:36, Vadim Sh. wrote:
> On 2014/12/30 22:54:01, nodir wrote:
> > I always thought multi-line lambdas do not satisfy our styleguide, and make
> > myself defining normals funcs
> 
> Never heard of that :)

https://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Lambda_Functions

> Generally I avoid line wrapping in lambdas, but if it syntactically correct
and
> shorter - why not?

I don't know why not

https://codereview.chromium.org/816433004/diff/20001/appengine/chrome_infra_p...
File appengine/chrome_infra_packages/cipd/impl.py (right):

https://codereview.chromium.org/816433004/diff/20001/appengine/chrome_infra_p...
appengine/chrome_infra_packages/cipd/impl.py:254: assert s.signature_algo ==
sig.signature_algo, (s, sig)
I am not sure why you assert this equality? I thought you should check it.
Figure signature_key, then hash_algo, digest, signature_algo JIC, and then
signature

[Vadim Sh., 2015-01-02 19:31:03]

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
File appengine/chrome_infra_packages/cipd/test/impl_test.py (right):

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/test/impl_test.py:176: expected = lambda
key: {
On 2015/01/02 19:13:04, nodir wrote:
> On 2014/12/31 01:27:36, Vadim Sh. wrote:
> > On 2014/12/30 22:54:01, nodir wrote:
> > > I always thought multi-line lambdas do not satisfy our styleguide, and
make
> > > myself defining normals funcs
> > 
> > Never heard of that :)
> 
>
https://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Lambda_Functions
> 
> > Generally I avoid line wrapping in lambdas, but if it syntactically correct
> and
> > shorter - why not?
> 
> I don't know why not

Arguably, lambda x: <dict> is one liner. I prefer to keep it a lambda, it's
quite readable and more concise.

https://codereview.chromium.org/816433004/diff/20001/appengine/chrome_infra_p...
File appengine/chrome_infra_packages/cipd/impl.py (right):

https://codereview.chromium.org/816433004/diff/20001/appengine/chrome_infra_p...
appengine/chrome_infra_packages/cipd/impl.py:254: assert s.signature_algo ==
sig.signature_algo, (s, sig)
On 2015/01/02 19:13:04, nodir wrote:
> I am not sure why you assert this equality? I thought you should check it.
> Figure signature_key, then hash_algo, digest, signature_algo JIC, and then
> signature

If signatures match, all arguments involved in making the signature must match
too. If they are not, something fishy is going on and I'd prefer request to die
with assert rather then to do something smarter.

[nodir, 2015-01-02 20:23:23]

lgtm

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
File appengine/chrome_infra_packages/cipd/test/impl_test.py (right):

https://codereview.chromium.org/816433004/diff/1/appengine/chrome_infra_packa...
appengine/chrome_infra_packages/cipd/test/impl_test.py:176: expected = lambda
key: {
On 2015/01/02 19:31:02, Vadim Sh. wrote:
> On 2015/01/02 19:13:04, nodir wrote:
> > On 2014/12/31 01:27:36, Vadim Sh. wrote:
> > > On 2014/12/30 22:54:01, nodir wrote:
> > > > I always thought multi-line lambdas do not satisfy our styleguide, and
> make
> > > > myself defining normals funcs
> > > 
> > > Never heard of that :)
> > 
> >
>
https://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Lambda_Functions
> > 
> > > Generally I avoid line wrapping in lambdas, but if it syntactically
correct
> > and
> > > shorter - why not?
> > 
> > I don't know why not
> 
> Arguably, lambda x: <dict> is one liner. I prefer to keep it a lambda, it's
> quite readable and more concise. 

Acknowledged.

https://codereview.chromium.org/816433004/diff/20001/appengine/chrome_infra_p...
File appengine/chrome_infra_packages/cipd/impl.py (right):

https://codereview.chromium.org/816433004/diff/20001/appengine/chrome_infra_p...
appengine/chrome_infra_packages/cipd/impl.py:254: assert s.signature_algo ==
sig.signature_algo, (s, sig)
On 2015/01/02 19:31:03, Vadim Sh. wrote:
> On 2015/01/02 19:13:04, nodir wrote:
> > I am not sure why you assert this equality? I thought you should check it.
> > Figure signature_key, then hash_algo, digest, signature_algo JIC, and then
> > signature
> 
> If signatures match, all arguments involved in making the signature must match
> too. If they are not, something fishy is going on and I'd prefer request to
die
> with assert rather then to do something smarter.

Acknowledged.

[Vadim Sh., 2015-01-02 20:50:29]

The CQ bit was checked by vadimsh@chromium.org

[commit-bot: I haz the power, 2015-01-02 20:50:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/816433004/20001

[commit-bot: I haz the power, 2015-01-02 20:53:09]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/infra/infra/+/9ea72bb7ae1e895bf5832b26414dd...