[cipd] Enable subdirs functionality.

[cipd] Enable subdirs functionality.

Final CL to enable subdir functionality via command line. This bumps the cipd
client version to 1.5, as it changes the output format of the `ensure` command.

R=dnj@chromium.org, vadimsh@chromium.org
BUG=663843
Review-Url: https://codereview.chromium.org/2661313003
Committed: https://github.com/luci/luci-go/commit/baeff8eb4d22068be213f22d8a092513485440d9

[iannucci, 2017-02-01 00:02:26]

PTAL: self-review comments on PS1

https://codereview.chromium.org/2661313003/diff/1/cipd/client/cipd/client.go
File cipd/client/cipd/client.go (right):

https://codereview.chromium.org/2661313003/diff/1/cipd/client/cipd/client.go#...
cipd/client/cipd/client.go:111: UserAgent = "cipd 1.5.0"
the 'ensure' command outputs different JSON now, hence the minor version bump.
Unless we're doing semver in which case this should be a major version bump?

https://codereview.chromium.org/2661313003/diff/1/cipd/client/cipd/client.go#...
cipd/client/cipd/client.go:236: type ActionMap map[string]*Actions
Basically the replacement for passing Actions around. This now maps subdir ->
Actions. The *Actions change was because the code does 
  `actions.member = append(actions.member, ...)`
stuff, and so the value needed to be modifiable.

https://codereview.chromium.org/2661313003/diff/1/cipd/client/cipd/client.go#...
cipd/client/cipd/client.go:239: func (am ActionMap) Log(ctx context.Context) {
This code used to be inline in the ensure function, but it was a cohesive enough
chunk that I moved it out.

https://codereview.chromium.org/2661313003/diff/1/cipd/client/cipd/client.go#...
cipd/client/cipd/client.go:1261: if len(aMap) == 0 {
buildActionPlan ensures that it will have len > 0 if and only if some subdirs
have non-empty actions

https://codereview.chromium.org/2661313003/diff/1/cipd/client/cipd/client.go#...
cipd/client/cipd/client.go:1273: hasErrors := false
this was easier (I think) than adding a hasErrors() method to ActionMap

https://codereview.chromium.org/2661313003/diff/1/cipd/client/cipd/client.go#...
cipd/client/cipd/client.go:1278: err = client.deployer.RemoveDeployed(ctx,
subdir, pin.PackageName)
keep an eye out for "" -> subdir changes.

https://codereview.chromium.org/2661313003/diff/1/cipd/client/cipd/client.go#...
cipd/client/cipd/client.go:1293: for subdir, actions := range aMap {
I basically do everything iteratively for now. We'll eventually want to
reorganize this to do:
  * buildActionPlan
  * fetch all new instances in parallel
  * validateState(allPins, fetchedInstances)
    - ensures that the instances in the proposed layout don't overlap each other
    - includes reading files from instance objects
  * remove files / deploy new files

https://codereview.chromium.org/2661313003/diff/1/cipd/client/cipd/client.go#...
cipd/client/cipd/client.go:1438: commonSubdirs.Iter(func(subdir string) bool {
I can simplify the set logic on all three of these a bit.

https://codereview.chromium.org/2661313003/diff/1/cipd/client/cipd/client.go#...
cipd/client/cipd/client.go:1462: if !a.Empty() {
this could happen if want == have

https://codereview.chromium.org/2661313003/diff/1/cipd/client/cipd/client.go#...
cipd/client/cipd/client.go:1468: if len(aMap) == 0 {
just normalize len == 0 to nil. It's cleaner.

https://codereview.chromium.org/2661313003/diff/1/cipd/client/cipd/client_tes...
File cipd/client/cipd/client_test.go (right):

https://codereview.chromium.org/2661313003/diff/1/cipd/client/cipd/client_tes...
cipd/client/cipd/client_test.go:643: shouldHaveContent := func(relPath
interface{}, data ...interface{}) string {
changed to a custom 'should' function so that the assertions get the correct
line numbers.

https://codereview.chromium.org/2661313003/diff/1/cipd/client/cipd/client_tes...
cipd/client/cipd/client_test.go:667: state :=
map[string][]local.PackageInstance{}
I changed this to be stateful in the test: makes it easier to write the tests
and it's local enough that I don't think it's confusing. Also reduces the
callEnsured boilerplate.

https://codereview.chromium.org/2661313003/diff/1/cipd/client/cipd/client_tes...
cipd/client/cipd/client_test.go:679: shouldBeDeployed := func(expect
interface{}, _ ...interface{}) string {
changed to a 'should' function so that assertions were less boilerplatey (and if
err != nil it gets the correct line numbers)

https://codereview.chromium.org/2661313003/diff/1/cipd/client/cmd/cipd/friend...
File cipd/client/cmd/cipd/friendly.go (left):

https://codereview.chromium.org/2661313003/diff/1/cipd/client/cmd/cipd/friend...
cipd/client/cmd/cipd/friendly.go:260: // List specific packages only.
I removed this functionality from 'installed' because it doesn't make a lot of
sense with subdirs

https://codereview.chromium.org/2661313003/diff/1/cipd/client/cmd/cipd/main.go
File cipd/client/cmd/cipd/main.go (right):

https://codereview.chromium.org/2661313003/diff/1/cipd/client/cmd/cipd/main.g...
cipd/client/cmd/cipd/main.go:209: func (c *Subcommand) doneWithPins(pins
[]pinInfo, err error) int {
there's now a difference between []pinInfo (e.g. remote service calls), and the
subdir map results (local operations like installed or ensure)

https://codereview.chromium.org/2661313003/diff/1/cipd/client/cmd/cipd/main.g...
cipd/client/cmd/cipd/main.go:609: if (hasPins || hasErrors) && (len(pinMap) > 1
|| subdir != "") {
this has the effect that printPinsAndError has the existing behavior when
printing the result of remote service calls and simple ensure'd installations
(only install packages in root), but also handles new subdir printouts
reasonably. This function should probably be split up, but it's easy enough to
refactor and this code is all a bit crunchy anyway.

https://codereview.chromium.org/2661313003/diff/1/cipd/client/cmd/cipd/main.g...
cipd/client/cmd/cipd/main.go:880: return c.doneWithPins(resolution, err)
this change wasn't needed

[Vadim Sh., 2017-02-01 03:05:25]

lgtm % sorting of actions

Also please verify that it can update stuff installed by an old client (e.g.
git_revision:9d08891c27ac35a4336abfb23c2beeb52d6f071c, as currently used by
Puppet). 

And verify that cipd recipe module doesn't try to parse ensure output JSON.

https://codereview.chromium.org/2661313003/diff/1/cipd/client/cipd/client.go
File cipd/client/cipd/client.go (right):

https://codereview.chromium.org/2661313003/diff/1/cipd/client/cipd/client.go#...
cipd/client/cipd/client.go:111: UserAgent = "cipd 1.5.0"
On 2017/02/01 00:02:25, iannucci wrote:
> the 'ensure' command outputs different JSON now, hence the minor version bump.
> Unless we're doing semver in which case this should be a major version bump?

1.5 is fine.

https://codereview.chromium.org/2661313003/diff/40001/cipd/client/cipd/client.go
File cipd/client/cipd/client.go (right):

https://codereview.chromium.org/2661313003/diff/40001/cipd/client/cipd/client...
cipd/client/cipd/client.go:1276: for subdir, actions := range aMap {
Can we sort aMap first? So if some weird shit happens, it happens reliably
everywhere (not depending on how map[...] was sorted by Go).

This is especially important until "TODO(iannucci): ensure that no packages
cross root boundaries" is implemented. If subdirs intersect, the outcome of
installation depends on the order.

https://codereview.chromium.org/2661313003/diff/40001/cipd/client/cipd/client...
cipd/client/cipd/client.go:1282: actions.Errors = append(actions.Errors,
ActionError{
I assume subdir is later seen in JSON error report as some key somewhere?

https://codereview.chromium.org/2661313003/diff/40001/cipd/client/cipd/client...
cipd/client/cipd/client.go:1427: // all removed subdirs
this will leave empty dirs, I think

e.g. if we had a subdir a/b/c/d/<pkg> and don't longer have it, we end up with
empty a/b/c/d

Not big deal, probably... I think we currently keep empty dirs also when
removing packages.

[iannucci, 2017-02-03 05:33:43]

https://codereview.chromium.org/2661313003/diff/40001/cipd/client/cipd/client.go
File cipd/client/cipd/client.go (right):

https://codereview.chromium.org/2661313003/diff/40001/cipd/client/cipd/client...
cipd/client/cipd/client.go:1276: for subdir, actions := range aMap {
On 2017/02/01 03:05:25, Vadim Sh. wrote:
> Can we sort aMap first? So if some weird shit happens, it happens reliably
> everywhere (not depending on how map[...] was sorted by Go).
> 
> This is especially important until "TODO(iannucci): ensure that no packages
> cross root boundaries" is implemented. If subdirs intersect, the outcome of
> installation depends on the order.

Done.

https://codereview.chromium.org/2661313003/diff/40001/cipd/client/cipd/client...
cipd/client/cipd/client.go:1282: actions.Errors = append(actions.Errors,
ActionError{
On 2017/02/01 03:05:25, Vadim Sh. wrote:
> I assume subdir is later seen in JSON error report as some key somewhere?

looks like that never got wired up, but since the returned action map is by
subdir, you could also print the errors by subdir too.

https://codereview.chromium.org/2661313003/diff/40001/cipd/client/cipd/client...
cipd/client/cipd/client.go:1427: // all removed subdirs
On 2017/02/01 03:05:25, Vadim Sh. wrote:
> this will leave empty dirs, I think
> 
> e.g. if we had a subdir a/b/c/d/<pkg> and don't longer have it, we end up with
> empty a/b/c/d
> 
> Not big deal, probably... I think we currently keep empty dirs also when
> removing packages.

So actually... 
https://github.com/luci/luci-go/blob/master/cipd/client/cipd/local/deployer.g...

:D

[Vadim Sh., 2017-02-03 05:37:02]

https://codereview.chromium.org/2661313003/diff/40001/cipd/client/cipd/client.go
File cipd/client/cipd/client.go (right):

https://codereview.chromium.org/2661313003/diff/40001/cipd/client/cipd/client...
cipd/client/cipd/client.go:1427: // all removed subdirs
On 2017/02/03 05:33:42, iannucci wrote:
> On 2017/02/01 03:05:25, Vadim Sh. wrote:
> > this will leave empty dirs, I think
> > 
> > e.g. if we had a subdir a/b/c/d/<pkg> and don't longer have it, we end up
with
> > empty a/b/c/d
> > 
> > Not big deal, probably... I think we currently keep empty dirs also when
> > removing packages.
> 
> So actually... 
>
https://github.com/luci/luci-go/blob/master/cipd/client/cipd/local/deployer.g...
> 
> :D

There it applies only to empty dirs in .cipd/pkgs/<index>/<sha>/* guts. The
whole <sha>/* gets nuked when the package is updated. So empty dirs at least do
not accumulate :)

[iannucci, 2017-02-03 05:38:33]

The CQ bit was checked by iannucci@chromium.org

[iannucci, 2017-02-03 05:38:34]

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/2661313003/#ps60001
(title: "iterate in order")

[commit-bot: I haz the power, 2017-02-03 05:38:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-03 05:40:08]

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

[commit-bot: I haz the power, 2017-02-03 05:40:08]

Try jobs failed on following builders:
  Luci-go Mac Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/341b4cf5d0054f10)
  Luci-go Presubmit on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/341b4cf5f807fc10)

[iannucci, 2017-02-03 05:51:47]

The CQ bit was checked by iannucci@chromium.org

[iannucci, 2017-02-03 05:51:48]

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/2661313003/#ps80001
(title: "Make golint happy")

[commit-bot: I haz the power, 2017-02-03 05:51:58]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-03 05:58:13]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1486101107759380,
"parent_rev": "0ea1a75e1637c226e9d156b73f83b6b3973379bb", "commit_rev":
"baeff8eb4d22068be213f22d8a092513485440d9"}

[commit-bot: I haz the power, 2017-02-03 05:58:16]

Description was changed from

==========
[cipd] Enable subdirs functionality.

Final CL to enable subdir functionality via command line. This bumps the cipd
client version to 1.5, as it changes the output format of the `ensure` command.

R=dnj@chromium.org, vadimsh@chromium.org
BUG=663843
==========

to

==========
[cipd] Enable subdirs functionality.

Final CL to enable subdir functionality via command line. This bumps the cipd
client version to 1.5, as it changes the output format of the `ensure` command.

R=dnj@chromium.org, vadimsh@chromium.org
BUG=663843

Review-Url: https://codereview.chromium.org/2661313003
Committed:
https://github.com/luci/luci-go/commit/baeff8eb4d22068be213f22d8a092513485440d9
==========

[commit-bot: I haz the power, 2017-02-03 05:58:17]

Committed patchset #5 (id:80001) as
https://github.com/luci/luci-go/commit/baeff8eb4d22068be213f22d8a092513485440d9