Add a package listing API to cipd.

Add a package listing API to cipd.

BUG=495976
Committed: https://chromium.googlesource.com/infra/infra/+/ee35f4ff955c449f796f1da186be98fbb817be6c

[estaab, 2015-06-19 19:19:16]

estaab@chromium.org changed reviewers:
+ vadimsh@chromium.org

[Vadim Sh., 2015-06-19 21:12:03]

looks good with some minor comments

Also what do you think about using structure of package paths in search, i.e.
make it look a bit more like file system: 
"cipd list infra/tools" => /package/search?prefix=infra/tools => returns
immediate children ["infra/tools/cipd"].
"cipd list infra/tools -recursive" =>
/package/search?prefix=infra/tools&recursive=1 => returns whole subtree.

Is there a value in supporting string based prefix search? Or rather do you seen
any use cases of "list" when a prefix string is not a "directory"
(e.g."infra/tools") but just a chunk of a package name (e.g. "infra/too")?

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

https://codereview.chromium.org/1194803002/diff/1/appengine/chrome_infra_pack...
appengine/chrome_infra_packages/cipd/api.py:553: # REVIEW: Should I be reusing
the /package path with an additional prefix
I think packages is ok. Consider also "GET /package/search?prefix=...". It will
be similar to existing "/instance/search?tag=..." call then. The idea here is
that we retrieve "search results" object, not a package. Thus "package/search"
noun, not "package".

But it doesn't really matter :)

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

https://codereview.chromium.org/1194803002/diff/1/appengine/chrome_infra_pack...
appengine/chrome_infra_packages/cipd/impl.py:186: ndb.AND(Package.key >=
ndb.Key(Package, prefix),
this thing may require additional composite index (in index.yaml). The simplest
way to find out is to deploy  it to chrome-infra-packages-dev.appspot.com (I'll
send you invite) and try the call.

https://codereview.chromium.org/1194803002/diff/1/appengine/chrome_infra_pack...
appengine/chrome_infra_packages/cipd/impl.py:189: for pkg in query:
use keys_only=True query and extract package_name from the key. Will be much
faster that way (index-only query).

https://codereview.chromium.org/1194803002/diff/1/appengine/chrome_infra_pack...
appengine/chrome_infra_packages/cipd/impl.py:191: return pkgs
return sorted(pkgs)

https://codereview.chromium.org/1194803002/diff/1/go/src/infra/tools/cipd/app...
File go/src/infra/tools/cipd/apps/cipd/main.go (right):

https://codereview.chromium.org/1194803002/diff/1/go/src/infra/tools/cipd/app...
go/src/infra/tools/cipd/apps/cipd/main.go:898: // 'pkg-list' subcommand.
pkg-* prefix is for subcommands that touch package files on disk. Just 'list'?

https://codereview.chromium.org/1194803002/diff/1/go/src/infra/tools/cipd/app...
go/src/infra/tools/cipd/apps/cipd/main.go:1036: cmdListPackages, // REVIEW:
Where should this actually go?
"High level commands" I think.

[estaab, 2015-06-20 02:25:25]

Ok, I changed this to make it act mostly like ls with a real filesystem at the
cost of a lot of complexity. :)  I think it behaves a lot better now though.

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

https://codereview.chromium.org/1194803002/diff/1/appengine/chrome_infra_pack...
appengine/chrome_infra_packages/cipd/api.py:553: # REVIEW: Should I be reusing
the /package path with an additional prefix
On 2015/06/19 21:12:03, Vadim Sh. wrote:
> I think packages is ok. Consider also "GET /package/search?prefix=...". It
will
> be similar to existing "/instance/search?tag=..." call then. The idea here is
> that we retrieve "search results" object, not a package. Thus "package/search"
> noun, not "package".
> 
> But it doesn't really matter :)

Done.

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

https://codereview.chromium.org/1194803002/diff/1/appengine/chrome_infra_pack...
appengine/chrome_infra_packages/cipd/impl.py:186: ndb.AND(Package.key >=
ndb.Key(Package, prefix),
On 2015/06/19 21:12:03, Vadim Sh. wrote:
> this thing may require additional composite index (in index.yaml). The
simplest
> way to find out is to deploy  it to
http://chrome-infra-packages-dev.appspot.com (I'll
> send you invite) and try the call.

Sounds good.

https://codereview.chromium.org/1194803002/diff/1/appengine/chrome_infra_pack...
appengine/chrome_infra_packages/cipd/impl.py:189: for pkg in query:
On 2015/06/19 21:12:03, Vadim Sh. wrote:
> use keys_only=True query and extract package_name from the key. Will be much
> faster that way (index-only query).

Done.

https://codereview.chromium.org/1194803002/diff/1/appengine/chrome_infra_pack...
appengine/chrome_infra_packages/cipd/impl.py:191: return pkgs
On 2015/06/19 21:12:03, Vadim Sh. wrote:
> return sorted(pkgs)

Done

https://codereview.chromium.org/1194803002/diff/1/go/src/infra/tools/cipd/app...
File go/src/infra/tools/cipd/apps/cipd/main.go (right):

https://codereview.chromium.org/1194803002/diff/1/go/src/infra/tools/cipd/app...
go/src/infra/tools/cipd/apps/cipd/main.go:898: // 'pkg-list' subcommand.
On 2015/06/19 21:12:03, Vadim Sh. wrote:
> pkg-* prefix is for subcommands that touch package files on disk. Just 'list'?


Done, using ls.

https://codereview.chromium.org/1194803002/diff/1/go/src/infra/tools/cipd/app...
go/src/infra/tools/cipd/apps/cipd/main.go:1036: cmdListPackages, // REVIEW:
Where should this actually go?
On 2015/06/19 21:12:03, Vadim Sh. wrote:
> "High level commands" I think.

Done.

[estaab, 2015-06-20 19:48:35]

One more thing: I also considered returning directories as a separate field in
the reply since the command line is probably the only client that will want to
merge them. I'm not tied to this design and I'm happy to change it as needed to
make it right.

[Vadim Sh., 2015-06-22 17:28:27]

you forgot to upload a new patch set

[estaab, 2015-06-22 17:56:45]

On 2015/06/22 17:28:27, Vadim Sh. wrote:
> you forgot to upload a new patch set

Ah, sorry about that. Actually, I'm not happy with that patch set so I'm going
to change it to return a directories list separately and move the merge logic to
the client side. It will be much cleaner. I'll send it out in a couple hours.

[estaab, 2015-06-22 23:01:05]

Uploaded the new patch set with package/directory split on the server side.

[Vadim Sh., 2015-06-23 00:08:35]

Looks good.

I think we should keep existing behavior of "package can be a dir" because I
believe ultimately it makes the data model simpler: there's only one kind of
objects (packages, that also sort of like directories at the same time) instead
of two (packages and, separately, directories). I believe for most use cases
directories are irrelevant, listing is the only place where it matters. In fact,
for most use cases hierarchical structure of package names is irrelevant.

Not sure what to do with the logic that auto-registers package on ACL
modification (e.g. autoregistered 'infra' package): 
a) Remove it? Then 'ls' output will be cleaner, but there would be no way to
list ACL-significant prefixes that doesn't have packages yet. (e.g. team X asks
us for a prefix, we make 'teams/X' prefix with X as owner and tell them to use
it (but do not upload packages ourselves)). Do we care about this use case?
b) Keep it, but hide non-leaf empty (no uploaded instances) packages from the
listing?
c) Teach "ls" to hide empty packages via new command line flag?
d) Do nothing :)

I think I'm in favor of "c)", but in a separate CL. Wdyt?

https://codereview.chromium.org/1194803002/diff/20001/appengine/chrome_infra_...
File appengine/chrome_infra_packages/app.yaml (right):

https://codereview.chromium.org/1194803002/diff/20001/appengine/chrome_infra_...
appengine/chrome_infra_packages/app.yaml:1: application: chrome-infra-packages
bring back -dev please

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

https://codereview.chromium.org/1194803002/diff/20001/appengine/chrome_infra_...
appengine/chrome_infra_packages/cipd/impl.py:171: """Tests is the path is under
the given directory.
typo: if

https://codereview.chromium.org/1194803002/diff/20001/appengine/chrome_infra_...
appengine/chrome_infra_packages/cipd/impl.py:176: directory: String, directory
the path should fall under.
nit: start arg desc with lower case letter

https://codereview.chromium.org/1194803002/diff/20001/appengine/chrome_infra_...
appengine/chrome_infra_packages/cipd/impl.py:184: logging.info('processing %s in
%s', path, directory)
remove logging if it doesn't provide any additional information. Once this code
is fully tested and stable, logs here would not be useful.

On GAE logs are quota-controlled non-infinite resource.

https://codereview.chromium.org/1194803002/diff/20001/appengine/chrome_infra_...
appengine/chrome_infra_packages/cipd/impl.py:230: for key in
query.iter(keys_only=True):
non-ancestor queries are eventually consistent: the query can occasionally lie
and return entities that do not match filters. I doubt it happens for this
particular query, but it's not hard to filter out false hits with
pkg.startswith(dir_path) check.

From https://cloud.google.com/appengine/docs/python/datastore/queries:
"""
In an eventually consistent query, the indexes used to gather the results are
also accessed with eventual consistency. Consequently, such queries may
sometimes return entities that no longer match the original query criteria.
"""

https://codereview.chromium.org/1194803002/diff/20001/appengine/chrome_infra_...
appengine/chrome_infra_packages/cipd/impl.py:236: if '/' in pkg and
len(dir_path) != len(pkg):
I don't understand len(dir_path) != len(pkg) condition. What does it guard from?

https://codereview.chromium.org/1194803002/diff/20001/appengine/chrome_infra_...
appengine/chrome_infra_packages/cipd/impl.py:239:
logging.info(str(['/'.join(parts[:n]) for n in xrange(1, len(parts))]))
remove logging, it's probably not helpful in released code

https://codereview.chromium.org/1194803002/diff/20001/appengine/chrome_infra_...
File appengine/chrome_infra_packages/cipd/test/impl_test.py (right):

https://codereview.chromium.org/1194803002/diff/20001/appengine/chrome_infra_...
appengine/chrome_infra_packages/cipd/test/impl_test.py:143: # support for being
both a package and directory.
If we do it, we have to do it correctly: package registration code must check
that new package name is not a subpath of some other package. It's hard to do
transactionally (i.e. with 100% certainty) with current data model.

What are consequences of "a directory is also a package" quirk now (besides
being a bit odd)?

https://codereview.chromium.org/1194803002/diff/20001/go/src/infra/tools/cipd...
File go/src/infra/tools/cipd/apps/cipd/main.go (right):

https://codereview.chromium.org/1194803002/diff/20001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/apps/cipd/main.go:45: maxPositionalCount int) bool {
nit: we don't limit go code to 80 chars. In this case I think it'll be better to
keep it on the same line (and deduplicating 'int'):

func ... minPositionalCount, maxPositionalCount int)

also replacing 'positional' with just 'pos' is also ok (e.g. minPosCount,
maxPosCount).

https://codereview.chromium.org/1194803002/diff/20001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/apps/cipd/main.go:319: // ListPackagesOptions mixin.
no need for separate struct if it's not going to be reused by multiple
subcommands.

https://codereview.chromium.org/1194803002/diff/20001/go/src/infra/tools/cipd...
File go/src/infra/tools/cipd/client.go (right):

https://codereview.chromium.org/1194803002/diff/20001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/client.go:292: for k, d := range dirs {
IIUC, that's the only place where dir vs pkg matter now?

How about embracing "dir can be a package" situation and just display it in UI
somehow. Show both dir (with "/") and package without ("/"). (I think it happens
now).

I know at least one example of filesystem-like datamodel where directories are
no special from leaf nodes: zookeeper. So it's not unheard of. 

My concern is that trying to keep directories as a special kind of entities can
lead to more complications than gains.

https://codereview.chromium.org/1194803002/diff/20001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/client.go:297: sort.Sort(sort.StringSlice(allPkgs))
sort.Strings(allPkgs)

https://codereview.chromium.org/1194803002/diff/20001/go/src/infra/tools/cipd...
File go/src/infra/tools/cipd/remote.go (right):

https://codereview.chromium.org/1194803002/diff/20001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/remote.go:470: func packageEndpoint(path string,
recursive bool) (string, error) {
nit: packageSearchEndpoint

[Vadim Sh., 2015-06-23 00:56:28]

After thinking a bit more about pkg vs dir problem: I think I'll remove a notion
of "empty package" (a package entity with no uploaded instances) to reduce WTF
factor. 

(Some terms I use below: 
 * ACL tree: set of PackageACL entities that for filesystem-like structure.
 * Package tree: set of Package entities that also for filesystem-like
structure. ACL tree is an overlay of top of Package tree.)

Currently empty package is used mostly for ACL purposes:
1) Creating a package requires higher lever of permissions than just uploading a
new instance to an existing package (OWNER vs WRITER). So in theory OWNER can
create an empty package and let WRITER to upload something to it. In practice
this feature is not used, so I think I'll just allow WRITERs to create new
packages. (and OWNERS will still be able modify ACLs).
2) Each entry in ACL tree has corresponding entry in package tree (even it if is
empty package). I think it might be simpler to get rid of this link and declare
that ACL tree and package tree have only names in common, and do not otherwise
share data. In particular, it means "ls" equivalent for ACL tree would need to
be implemented separately, if ever needed.

Note that it doesn't eliminate "package is also a dir" problem. It removes most
occurrences of empty dir-like packages though.

[Vadim Sh., 2015-06-23 00:59:01]

On 2015/06/23 00:56:28, Vadim Sh. wrote:
> After thinking a bit more about pkg vs dir problem: I think I'll remove a
notion
> of "empty package" (a package entity with no uploaded instances) to reduce WTF
> factor. 
> 
> (Some terms I use below: 
>  * ACL tree: set of PackageACL entities that for filesystem-like structure.
>  * Package tree: set of Package entities that also for filesystem-like
> structure. ACL tree is an overlay of top of Package tree.)
> 
* for => form 
* overlay on top of
:( sorry about typos
and by "filesystem-like structure" I meant just hierarchal names.

> Currently empty package is used mostly for ACL purposes:
> 1) Creating a package requires higher lever of permissions than just uploading
a
> new instance to an existing package (OWNER vs WRITER). So in theory OWNER can
> create an empty package and let WRITER to upload something to it. In practice
> this feature is not used, so I think I'll just allow WRITERs to create new
> packages. (and OWNERS will still be able modify ACLs).
> 2) Each entry in ACL tree has corresponding entry in package tree (even it if
is
> empty package). I think it might be simpler to get rid of this link and
declare
> that ACL tree and package tree have only names in common, and do not otherwise
> share data. In particular, it means "ls" equivalent for ACL tree would need to
> be implemented separately, if ever needed.
> 
> Note that it doesn't eliminate "package is also a dir" problem. It removes
most
> occurrences of empty dir-like packages though.

[estaab, 2015-06-23 02:26:14]

Done with comments. And I agree with your writeup - having separate trees that
only share names sounds perfect.

https://codereview.chromium.org/1194803002/diff/20001/appengine/chrome_infra_...
File appengine/chrome_infra_packages/app.yaml (right):

https://codereview.chromium.org/1194803002/diff/20001/appengine/chrome_infra_...
appengine/chrome_infra_packages/app.yaml:1: application: chrome-infra-packages
On 2015/06/23 00:08:34, Vadim Sh. wrote:
> bring back -dev please

Done. Sorry that wasn't supposed to go in the CL.

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

https://codereview.chromium.org/1194803002/diff/20001/appengine/chrome_infra_...
appengine/chrome_infra_packages/cipd/impl.py:171: """Tests is the path is under
the given directory.
On 2015/06/23 00:08:34, Vadim Sh. wrote:
> typo: if

Done.

https://codereview.chromium.org/1194803002/diff/20001/appengine/chrome_infra_...
appengine/chrome_infra_packages/cipd/impl.py:176: directory: String, directory
the path should fall under.
On 2015/06/23 00:08:35, Vadim Sh. wrote:
> nit: start arg desc with lower case letter

Done.

https://codereview.chromium.org/1194803002/diff/20001/appengine/chrome_infra_...
appengine/chrome_infra_packages/cipd/impl.py:184: logging.info('processing %s in
%s', path, directory)
On 2015/06/23 00:08:34, Vadim Sh. wrote:
> remove logging if it doesn't provide any additional information. Once this
code
> is fully tested and stable, logs here would not be useful.
> 
> On GAE logs are quota-controlled non-infinite resource.

Sorry, also not supposed to be in the CL.

https://codereview.chromium.org/1194803002/diff/20001/appengine/chrome_infra_...
appengine/chrome_infra_packages/cipd/impl.py:230: for key in
query.iter(keys_only=True):
On 2015/06/23 00:08:35, Vadim Sh. wrote:
> non-ancestor queries are eventually consistent: the query can occasionally lie
> and return entities that do not match filters. I doubt it happens for this
> particular query, but it's not hard to filter out false hits with
> pkg.startswith(dir_path) check.
> 
> From https://cloud.google.com/appengine/docs/python/datastore/queries:
> """
> In an eventually consistent query, the indexes used to gather the results are
> also accessed with eventual consistency. Consequently, such queries may
> sometimes return entities that no longer match the original query criteria.
> """

Good to know, done. I couldn't think of an easy way to test this but it's
probably not too important. The no cover pragma is ugly, though.

https://codereview.chromium.org/1194803002/diff/20001/appengine/chrome_infra_...
appengine/chrome_infra_packages/cipd/impl.py:236: if '/' in pkg and
len(dir_path) != len(pkg):
On 2015/06/23 00:08:35, Vadim Sh. wrote:
> I don't understand len(dir_path) != len(pkg) condition. What does it guard
from?

Removed, this was from some older logic.

https://codereview.chromium.org/1194803002/diff/20001/appengine/chrome_infra_...
appengine/chrome_infra_packages/cipd/impl.py:239:
logging.info(str(['/'.join(parts[:n]) for n in xrange(1, len(parts))]))
On 2015/06/23 00:08:34, Vadim Sh. wrote:
> remove logging, it's probably not helpful in released code

Done, also a mistake.

https://codereview.chromium.org/1194803002/diff/20001/appengine/chrome_infra_...
File appengine/chrome_infra_packages/cipd/test/impl_test.py (right):

https://codereview.chromium.org/1194803002/diff/20001/appengine/chrome_infra_...
appengine/chrome_infra_packages/cipd/test/impl_test.py:143: # support for being
both a package and directory.
On 2015/06/23 00:08:35, Vadim Sh. wrote:
> If we do it, we have to do it correctly: package registration code must check
> that new package name is not a subpath of some other package. It's hard to do
> transactionally (i.e. with 100% certainty) with current data model.
> 
> What are consequences of "a directory is also a package" quirk now (besides
> being a bit odd)?

It should be ok. I changed the comment.

https://codereview.chromium.org/1194803002/diff/20001/go/src/infra/tools/cipd...
File go/src/infra/tools/cipd/apps/cipd/main.go (right):

https://codereview.chromium.org/1194803002/diff/20001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/apps/cipd/main.go:45: maxPositionalCount int) bool {
On 2015/06/23 00:08:35, Vadim Sh. wrote:
> nit: we don't limit go code to 80 chars. In this case I think it'll be better
to
> keep it on the same line (and deduplicating 'int'):
> 
> func ... minPositionalCount, maxPositionalCount int)
> 
> also replacing 'positional' with just 'pos' is also ok (e.g. minPosCount,
> maxPosCount). 

Done.

https://codereview.chromium.org/1194803002/diff/20001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/apps/cipd/main.go:319: // ListPackagesOptions mixin.
On 2015/06/23 00:08:35, Vadim Sh. wrote:
> no need for separate struct if it's not going to be reused by multiple
> subcommands.

Done.

https://codereview.chromium.org/1194803002/diff/20001/go/src/infra/tools/cipd...
File go/src/infra/tools/cipd/client.go (right):

https://codereview.chromium.org/1194803002/diff/20001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/client.go:292: for k, d := range dirs {
On 2015/06/23 00:08:35, Vadim Sh. wrote:
> IIUC, that's the only place where dir vs pkg matter now?
> 
> How about embracing "dir can be a package" situation and just display it in UI
> somehow. Show both dir (with "/") and package without ("/"). (I think it
happens
> now).
> 
> I know at least one example of filesystem-like datamodel where directories are
> no special from leaf nodes: zookeeper. So it's not unheard of. 
> 
> My concern is that trying to keep directories as a special kind of entities
can
> lead to more complications than gains.

Sounds fine, we show both now.

https://codereview.chromium.org/1194803002/diff/20001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/client.go:297: sort.Sort(sort.StringSlice(allPkgs))
On 2015/06/23 00:08:35, Vadim Sh. wrote:
> sort.Strings(allPkgs)

Done.

https://codereview.chromium.org/1194803002/diff/20001/go/src/infra/tools/cipd...
File go/src/infra/tools/cipd/remote.go (right):

https://codereview.chromium.org/1194803002/diff/20001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/remote.go:470: func packageEndpoint(path string,
recursive bool) (string, error) {
On 2015/06/23 00:08:35, Vadim Sh. wrote:
> nit: packageSearchEndpoint

Done.

[Vadim Sh., 2015-06-23 02:36:09]

lgtm, thanks :)

Once it lands, refetch it from origin/master and deploy to -dev and later to
main one:
$ git fetch origin/master
$ git checkout origin/master
$ cd appengine/chrome_infra_packages
$ ./tools/gae upload -A chrome-infra-packages-dev -x
$ ./tools/gae upload -A chrome-infra-packages -x

(origin/master is important, otherwise version name of GAE app will have
"-tainted" suffix, meaning it doesn't match what's in source control).
(-x means "switch current version to just uploaded one").

[estaab, 2015-06-23 02:42:45]

The CQ bit was checked by estaab@chromium.org

[commit-bot: I haz the power, 2015-06-23 02:43:04]

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

[commit-bot: I haz the power, 2015-06-23 02:44:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-23 02:44:52]

Try jobs failed on following builders:
  infra_tester on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/infra_tester/bu...)

[estaab, 2015-06-23 03:33:08]

The CQ bit was checked by estaab@chromium.org

[estaab, 2015-06-23 03:33:08]

The patchset sent to the CQ was uploaded after l-g-t-m from vadimsh@chromium.org
Link to the patchset: https://codereview.chromium.org/1194803002/#ps60001
(title: " ")

[commit-bot: I haz the power, 2015-06-23 03:33:14]

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

[commit-bot: I haz the power, 2015-06-23 03:37:51]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/infra/infra/+/ee35f4ff955c449f796f1da186be9...