Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1358533003:
cipd: Implement 'init', 'install' and 'installed' subcommands. (Closed)
Descriptioncipd: Implement 'init', 'install' and 'installed' subcommands.
* 'init' create .cipd/* directory with some minimal config.
* 'install' installs a package (auto-calling 'init' if appropriate).
* 'installed' lists installed packages.
Example usage:
$ mkdir stuff && cd stuff/
$ cipd install infra/infra_python/mac-amd64-mac10_10
Site root initialized at /.../stuff.
Installing infra/infra_python/mac-amd64-mac10_10 (version "latest")...
...
Package infra/infra_python/mac-amd64-mac10_10 is now tracking "latest".
Packages:
infra/infra_python/mac-amd64-mac10_10:472bf2f3daed7c92f708d42207ec7270d8bfe01a (tracking "latest")
$ cipd installed
Packages:
infra/infra_python/mac-amd64-mac10_10:472bf2f3daed7c92f708d42207ec7270d8bfe01a (tracking "latest")
R=nodir@chromium.org
TEST=manual
BUG=
Committed: https://chromium.googlesource.com/infra/infra/+/7bf444f470080f76364d1ed3a5a98314c0ee3f1f
Patch Set 1 #
Total comments: 30
Patch Set 2 : rebase #Patch Set 3 : #
Total comments: 9
Patch Set 4 : reply to comments #Patch Set 5 : rename pinOrError to pinInfo #
Total comments: 14
Patch Set 6 : #
Dependent Patchsets: Messages
Total messages: 19 (4 generated)
PTAL I plan to implement some high level integration test (in python probably) that will test client against devserver backend (or probably chrome-infra-packages-dev.appspot.com, since mocking Google Storage is hard). It will run all workflows CIPD is used in (creating packages, using "ensure", using "install", etc).
tandrii@chromium.org changed reviewers: + tandrii@chromium.org
I'm perf-fatigued, so I just left some comments, not final verdict. Still +1 for "friendly" commands. On 2015/09/19 04:35:35, Vadim Sh. wrote: > PTAL > > I plan to implement some high level integration test (in python probably) that > will test client against devserver backend (or probably > http://chrome-infra-packages-dev.appspot.com, since mocking Google Storage is hard). > > It will run all workflows CIPD is used in (creating packages, using "ensure", > using "install", etc). How about creating a cipd test recipe? Recipe step's test_data will be essentially expected output, which is easy to assert immediately, wouldn't that be awesome? Two downsides I see so far: * recipe has to be in build/, but it will soon be possible to move it back to infra/infra! * this is unusual, so will require a readme. (Or am I hallucinating? Harsh critic is welcomed). in_reply_to: 5649050225344512 send_mail: 1 subject: cipd: Implement 'init', 'install' and 'installed' subcommands. https://codereview.chromium.org/1358533003/diff/1/go/src/infra/tools/cipd/app... File go/src/infra/tools/cipd/apps/cipd/friendly.go (right): https://codereview.chromium.org/1358533003/diff/1/go/src/infra/tools/cipd/app... go/src/infra/tools/cipd/apps/cipd/friendly.go:26: func findSiteRoot(p string) string { Good! I coded this for something already... https://codereview.chromium.org/1358533003/diff/1/go/src/infra/tools/cipd/app... go/src/infra/tools/cipd/apps/cipd/friendly.go:128: // Is siteRoot is "", will find a site root based on current directory, s/Is/If https://codereview.chromium.org/1358533003/diff/1/go/src/infra/tools/cipd/app... go/src/infra/tools/cipd/apps/cipd/friendly.go:216: if !force { This is extra meaning for force flag then: doc above says just about non-empty dir, but here it's about creating .cipd inside a tree with .cipd .
https://codereview.chromium.org/1358533003/diff/1/go/src/infra/tools/cipd/app... File go/src/infra/tools/cipd/apps/cipd/friendly.go (right): https://codereview.chromium.org/1358533003/diff/1/go/src/infra/tools/cipd/app... go/src/infra/tools/cipd/apps/cipd/friendly.go:32: if parent == p { this seems to be undocumented behavior. Works on linux, but I cannot check on windows https://codereview.chromium.org/1358533003/diff/1/go/src/infra/tools/cipd/app... go/src/infra/tools/cipd/apps/cipd/friendly.go:130: func doInSiteRoot(siteRoot string, cb siteRootCB) error { I think just returning siteRoot and cfg would be simpler https://codereview.chromium.org/1358533003/diff/1/go/src/infra/tools/cipd/app... go/src/infra/tools/cipd/apps/cipd/friendly.go:146: func doInSiteRootWithClient(siteRoot string, authFlags authcli.Flags, cb siteRootWithClientCB) error { I think translating this to (siteConfig) func createClient() (cipd.Client, err) would be simpler https://codereview.chromium.org/1358533003/diff/1/go/src/infra/tools/cipd/app... go/src/infra/tools/cipd/apps/cipd/friendly.go:170: c.Flags.StringVar(&c.rootDir, "root", "", "Path to an installation site root directory.") optional: use "." as default so it is obvious for the user that it defaults to the current dir, when they run cipd install -help https://codereview.chromium.org/1358533003/diff/1/go/src/infra/tools/cipd/app... go/src/infra/tools/cipd/apps/cipd/friendly.go:215: msg := fmt.Sprintf("directory %s is already inside a site root (%s)", rootDir, existing) Is json-output=- only for debugging? Otherwise Fprintf to stderr here and below https://codereview.chromium.org/1358533003/diff/1/go/src/infra/tools/cipd/app... go/src/infra/tools/cipd/apps/cipd/friendly.go:227: if len(items) != 0 { what if err != nil? https://codereview.chromium.org/1358533003/diff/1/go/src/infra/tools/cipd/app... go/src/infra/tools/cipd/apps/cipd/friendly.go:309: pin, err := client.ResolveVersion(pkgName, version) could use cfg.makeCipdClient to get client https://codereview.chromium.org/1358533003/diff/1/go/src/infra/tools/cipd/app... go/src/infra/tools/cipd/apps/cipd/friendly.go:318: skipInstall := false I'd prefer positive name: forceInstall := true https://codereview.chromium.org/1358533003/diff/1/go/src/infra/tools/cipd/app... go/src/infra/tools/cipd/apps/cipd/friendly.go:329: if !skipInstall { negative on negative https://codereview.chromium.org/1358533003/diff/1/go/src/infra/tools/cipd/app... File go/src/infra/tools/cipd/apps/cipd/main.go (right): https://codereview.chromium.org/1358533003/diff/1/go/src/infra/tools/cipd/app... go/src/infra/tools/cipd/apps/cipd/main.go:179: // doneWithPins is a handy shortcut that prints array of pinOrError and deduces it is a slice, but whatever https://codereview.chromium.org/1358533003/diff/1/go/src/infra/tools/cipd/app... go/src/infra/tools/cipd/apps/cipd/main.go:796: ShortDesc: "queries backend for a list of matching packages", looks like mentioning impl details. I'd move them to longdesc and leave shortdesc intact
ptal refactored to be much cleaner https://codereview.chromium.org/1358533003/diff/1/go/src/infra/tools/cipd/app... File go/src/infra/tools/cipd/apps/cipd/friendly.go (right): https://codereview.chromium.org/1358533003/diff/1/go/src/infra/tools/cipd/app... go/src/infra/tools/cipd/apps/cipd/friendly.go:32: if parent == p { On 2015/09/21 22:26:58, nodir wrote: > this seems to be undocumented behavior. Works on linux, but I cannot check on > windows Why check if you have the source code? :) https://golang.org/src/path/filepath/path.go?s=13086:13114#L440 The documented bit is "The returned path does not end in a separator unless it is the root directory.", so I'll use it instead, but it is less readable imho. https://codereview.chromium.org/1358533003/diff/1/go/src/infra/tools/cipd/app... go/src/infra/tools/cipd/apps/cipd/friendly.go:128: // Is siteRoot is "", will find a site root based on current directory, On 2015/09/21 13:47:41, tandrii(chromium) wrote: > s/Is/If Done. https://codereview.chromium.org/1358533003/diff/1/go/src/infra/tools/cipd/app... go/src/infra/tools/cipd/apps/cipd/friendly.go:130: func doInSiteRoot(siteRoot string, cb siteRootCB) error { On 2015/09/21 22:26:58, nodir wrote: > I think just returning siteRoot and cfg would be simpler Done. Not sure why I went callback-warrior road. Probably overexposure to Javascript. https://codereview.chromium.org/1358533003/diff/1/go/src/infra/tools/cipd/app... go/src/infra/tools/cipd/apps/cipd/friendly.go:146: func doInSiteRootWithClient(siteRoot string, authFlags authcli.Flags, cb siteRootWithClientCB) error { On 2015/09/21 22:26:59, nodir wrote: > I think translating this to > > (siteConfig) func createClient() (cipd.Client, err) > > would be simpler client needs authFlags. Moved into separate initClient() method. https://codereview.chromium.org/1358533003/diff/1/go/src/infra/tools/cipd/app... go/src/infra/tools/cipd/apps/cipd/friendly.go:170: c.Flags.StringVar(&c.rootDir, "root", "", "Path to an installation site root directory.") On 2015/09/21 22:26:59, nodir wrote: > optional: use "." as default so it is obvious for the user that it defaults to > the current dir, when they run cipd install -help Done. https://codereview.chromium.org/1358533003/diff/1/go/src/infra/tools/cipd/app... go/src/infra/tools/cipd/apps/cipd/friendly.go:215: msg := fmt.Sprintf("directory %s is already inside a site root (%s)", rootDir, existing) On 2015/09/21 22:26:59, nodir wrote: > Is json-output=- only for debugging? Otherwise Fprintf to stderr here and below Error printing is done in done(). https://codereview.chromium.org/1358533003/diff/1/go/src/infra/tools/cipd/app... go/src/infra/tools/cipd/apps/cipd/friendly.go:216: if !force { On 2015/09/21 13:47:41, tandrii(chromium) wrote: > This is extra meaning for force flag then: doc above says just about non-empty > dir, but here it's about creating .cipd inside a tree with .cipd . Updated flag help string. https://codereview.chromium.org/1358533003/diff/1/go/src/infra/tools/cipd/app... go/src/infra/tools/cipd/apps/cipd/friendly.go:227: if len(items) != 0 { On 2015/09/21 22:26:59, nodir wrote: > what if err != nil? err != nil here only if os.IsNotExists(err) is true, meaning directory doesn't exist, which is ok. See 'if' two lines above :) https://codereview.chromium.org/1358533003/diff/1/go/src/infra/tools/cipd/app... go/src/infra/tools/cipd/apps/cipd/friendly.go:309: pin, err := client.ResolveVersion(pkgName, version) On 2015/09/21 22:26:58, nodir wrote: > could use cfg.makeCipdClient to get client did something else https://codereview.chromium.org/1358533003/diff/1/go/src/infra/tools/cipd/app... go/src/infra/tools/cipd/apps/cipd/friendly.go:318: skipInstall := false On 2015/09/21 22:26:59, nodir wrote: > I'd prefer positive name: forceInstall := true Done. https://codereview.chromium.org/1358533003/diff/1/go/src/infra/tools/cipd/app... go/src/infra/tools/cipd/apps/cipd/friendly.go:329: if !skipInstall { On 2015/09/21 22:26:58, nodir wrote: > negative on negative Done. https://codereview.chromium.org/1358533003/diff/1/go/src/infra/tools/cipd/app... File go/src/infra/tools/cipd/apps/cipd/main.go (right): https://codereview.chromium.org/1358533003/diff/1/go/src/infra/tools/cipd/app... go/src/infra/tools/cipd/apps/cipd/main.go:179: // doneWithPins is a handy shortcut that prints array of pinOrError and deduces On 2015/09/21 22:26:59, nodir wrote: > it is a slice, but whatever renamed to "list" https://codereview.chromium.org/1358533003/diff/1/go/src/infra/tools/cipd/app... go/src/infra/tools/cipd/apps/cipd/main.go:796: ShortDesc: "queries backend for a list of matching packages", On 2015/09/21 22:26:59, nodir wrote: > looks like mentioning impl details. I'd move them to longdesc and leave > shortdesc intact Done. I wanted to clarify that it lists packages on the backend, no ones installed locally.
https://codereview.chromium.org/1358533003/diff/1/go/src/infra/tools/cipd/app... File go/src/infra/tools/cipd/apps/cipd/main.go (right): https://codereview.chromium.org/1358533003/diff/1/go/src/infra/tools/cipd/app... go/src/infra/tools/cipd/apps/cipd/main.go:796: ShortDesc: "queries backend for a list of matching packages", On 2015/09/29 01:43:34, Vadim Sh. wrote: > On 2015/09/21 22:26:59, nodir wrote: > > looks like mentioning impl details. I'd move them to longdesc and leave > > shortdesc intact > > Done. I wanted to clarify that it lists packages on the backend, no ones > installed locally. weak suggestion: maybe then say in short version "on the server" (since this is cipd-client) https://codereview.chromium.org/1358533003/diff/40001/go/src/infra/tools/cipd... File go/src/infra/tools/cipd/apps/cipd/friendly.go (right): https://codereview.chromium.org/1358533003/diff/40001/go/src/infra/tools/cipd... go/src/infra/tools/cipd/apps/cipd/friendly.go:268: // version. On errors both pinOrError (its error part) and error are returned. i find this awkward because you end up just duplicating error twice on several lines. It this really just to avoid doing extra conversion before calling c.done? https://codereview.chromium.org/1358533003/diff/40001/go/src/infra/tools/cipd... go/src/infra/tools/cipd/apps/cipd/friendly.go:280: err = errors.New("client is not initialized") i find this line here confusing. I think it fits best at the very top as early exit, sicne it doesn't need anything beyond pkgName, which is func arg. And then I think you can just have pin, err := site.client.ResolveVersion(pkgName, version) if err! = nil{ ... https://codereview.chromium.org/1358533003/diff/40001/go/src/infra/tools/cipd... go/src/infra/tools/cipd/apps/cipd/friendly.go:375: c.Flags.BoolVar(&c.force, "force", false, "Create the site root even if the directory is not empty or already under another site root directory.") Thanks. I'd not say this is immediately clear to me, but after thinking I can't come up with better way to put it. https://codereview.chromium.org/1358533003/diff/40001/go/src/infra/tools/cipd... go/src/infra/tools/cipd/apps/cipd/friendly.go:464: return c.done(site.installPackage(pkgName, version, c.force)) I'd just write this here: pin, err = installPackage(pkgName, version, c.force) return c.done(PinOrError{pin,error}, err) and now that I think of it, I don't get why do you want to spit out error twice into json output.
ptal https://codereview.chromium.org/1358533003/diff/40001/go/src/infra/tools/cipd... File go/src/infra/tools/cipd/apps/cipd/friendly.go (right): https://codereview.chromium.org/1358533003/diff/40001/go/src/infra/tools/cipd... go/src/infra/tools/cipd/apps/cipd/friendly.go:268: // version. On errors both pinOrError (its error part) and error are returned. On 2015/09/29 14:21:25, tandrii(chromium) wrote: > i find this awkward because you end up just duplicating error twice on several > lines. It this really just to avoid doing extra conversion before calling > c.done? They idea was to be able to put multiple pinOrError in an array when installing a bunch of packages at once. Changed to return error or pinOrError, but not both. https://codereview.chromium.org/1358533003/diff/40001/go/src/infra/tools/cipd... go/src/infra/tools/cipd/apps/cipd/friendly.go:280: err = errors.New("client is not initialized") On 2015/09/29 14:21:26, tandrii(chromium) wrote: > i find this line here confusing. I think it fits best at the very top as early > exit, sicne it doesn't need anything beyond pkgName, which is func arg. > And then I think you can just have > > pin, err := site.client.ResolveVersion(pkgName, version) > if err! = nil{ ... Done. https://codereview.chromium.org/1358533003/diff/40001/go/src/infra/tools/cipd... go/src/infra/tools/cipd/apps/cipd/friendly.go:464: return c.done(site.installPackage(pkgName, version, c.force)) On 2015/09/29 14:21:25, tandrii(chromium) wrote: > I'd just write this here: > > pin, err = installPackage(pkgName, version, c.force) > return c.done(PinOrError{pin,error}, err) > pinOrError actually is triple (pin, err, tracked version) now. I should probably rename it to something else... > and now that I think of it, I don't get why do you want to spit out error twice > into json output. No longer doing that.
tandrii@google.com changed reviewers: + tandrii@google.com
i lgtm the way it is now, but i'd prefer you get one from nodir too :) https://codereview.chromium.org/1358533003/diff/40001/go/src/infra/tools/cipd... File go/src/infra/tools/cipd/apps/cipd/friendly.go (right): https://codereview.chromium.org/1358533003/diff/40001/go/src/infra/tools/cipd... go/src/infra/tools/cipd/apps/cipd/friendly.go:268: // version. On errors both pinOrError (its error part) and error are returned. On 2015/09/29 19:13:27, Vadim Sh. wrote: > On 2015/09/29 14:21:25, tandrii(chromium) wrote: > > i find this awkward because you end up just duplicating error twice on several > > lines. It this really just to avoid doing extra conversion before calling > > c.done? > > They idea was to be able to put multiple pinOrError in an array when installing > a bunch of packages at once. Ah, yes, that's what I guessed, but with one package it's was weird. https://codereview.chromium.org/1358533003/diff/40001/go/src/infra/tools/cipd... go/src/infra/tools/cipd/apps/cipd/friendly.go:464: return c.done(site.installPackage(pkgName, version, c.force)) On 2015/09/29 19:13:27, Vadim Sh. wrote: > On 2015/09/29 14:21:25, tandrii(chromium) wrote: > > I'd just write this here: > > > > pin, err = installPackage(pkgName, version, c.force) > > return c.done(PinOrError{pin,error}, err) > > > pinOrError actually is triple (pin, err, tracked version) now. I should probably > rename it to something else... > > > and now that I think of it, I don't get why do you want to spit out error > twice > > into json output. > > No longer doing that. Acknowledged. https://codereview.chromium.org/1358533003/diff/80001/go/src/infra/tools/cipd... File go/src/infra/tools/cipd/apps/cipd/main.go (right): https://codereview.chromium.org/1358533003/diff/80001/go/src/infra/tools/cipd... go/src/infra/tools/cipd/apps/cipd/main.go:48: type pinInfo struct { Just realized that Pkg is duplicated here, can instanceId be stored as pointer to indicate nil instead of Pin? (though empty string would probably be just as good).
https://codereview.chromium.org/1358533003/diff/80001/go/src/infra/tools/cipd... File go/src/infra/tools/cipd/apps/cipd/main.go (right): https://codereview.chromium.org/1358533003/diff/80001/go/src/infra/tools/cipd... go/src/infra/tools/cipd/apps/cipd/main.go:48: type pinInfo struct { On 2015/09/29 22:48:20, tandrii_google wrote: > Just realized that Pkg is duplicated here, can instanceId be stored as pointer > to indicate nil instead of Pin? (though empty string would probably be just as > good). I like common.Pin to be represented uniformly as dict with two keys in JSON everywhere. When error occurs, there's no common.Pin, but I still need to specify package name. Adding error inside common.Pin will make lots of other API's uglier.
https://codereview.chromium.org/1358533003/diff/80001/go/src/infra/tools/cipd... File go/src/infra/tools/cipd/apps/cipd/main.go (right): https://codereview.chromium.org/1358533003/diff/80001/go/src/infra/tools/cipd... go/src/infra/tools/cipd/apps/cipd/main.go:48: type pinInfo struct { On 2015/09/30 00:23:09, Vadim Sh. wrote: > On 2015/09/29 22:48:20, tandrii_google wrote: > > Just realized that Pkg is duplicated here, can instanceId be stored as pointer > > to indicate nil instead of Pin? (though empty string would probably be just as > > good). > > I like common.Pin to be represented uniformly as dict with two keys in JSON > everywhere. When error occurs, there's no common.Pin, but I still need to > specify package name. > > Adding error inside common.Pin will make lots of other API's uglier. Ah, json - that makes sense.
https://chromiumcodereview-hr.appspot.com/1358533003/diff/1/go/src/infra/tool... File go/src/infra/tools/cipd/apps/cipd/friendly.go (right): https://chromiumcodereview-hr.appspot.com/1358533003/diff/1/go/src/infra/tool... go/src/infra/tools/cipd/apps/cipd/friendly.go:215: msg := fmt.Sprintf("directory %s is already inside a site root (%s)", rootDir, existing) On 2015/09/29 01:43:33, Vadim Sh. wrote: > On 2015/09/21 22:26:59, nodir wrote: > > Is json-output=- only for debugging? Otherwise Fprintf to stderr here and > below > > Error printing is done in done(). Acknowledged. https://chromiumcodereview-hr.appspot.com/1358533003/diff/1/go/src/infra/tool... go/src/infra/tools/cipd/apps/cipd/friendly.go:227: if len(items) != 0 { On 2015/09/29 01:43:33, Vadim Sh. wrote: > On 2015/09/21 22:26:59, nodir wrote: > > what if err != nil? > > err != nil here only if os.IsNotExists(err) is true, meaning directory doesn't > exist, which is ok. See 'if' two lines above :) Ack. I read them incorrectly https://chromiumcodereview-hr.appspot.com/1358533003/diff/80001/go/src/infra/... File go/src/infra/tools/cipd/apps/cipd/friendly.go (right): https://chromiumcodereview-hr.appspot.com/1358533003/diff/80001/go/src/infra/... go/src/infra/tools/cipd/apps/cipd/friendly.go:39: if parent[len(parent)-1] == filepath.Separator { according to the comment, it can be C:\ https://chromiumcodereview-hr.appspot.com/1358533003/diff/80001/go/src/infra/... go/src/infra/tools/cipd/apps/cipd/friendly.go:136: // files of directories, just reads what's on disk. or directories https://chromiumcodereview-hr.appspot.com/1358533003/diff/80001/go/src/infra/... go/src/infra/tools/cipd/apps/cipd/friendly.go:178: fmt.Printf("Warning: %s.\n", msg) this kind of stuff should be printed to stderr if you have a lot of calls like this: warning := log.New(os.Stderr, "Warning: ", 0) ... warning.Println(msg) https://chromiumcodereview-hr.appspot.com/1358533003/diff/80001/go/src/infra/... go/src/infra/tools/cipd/apps/cipd/friendly.go:194: // only once. Use it directly then via site.client. s/then/than/ or s/via/use/ https://chromiumcodereview-hr.appspot.com/1358533003/diff/80001/go/src/infra/... go/src/infra/tools/cipd/apps/cipd/friendly.go:234: cpy := pin `pin` is a copy anyway because it is stored on stack https://play.golang.org/p/yrb3uSTbqk a for-range loop with value variable clones the entire slice/(map values)
https://chromiumcodereview.appspot.com/1358533003/diff/80001/go/src/infra/too... File go/src/infra/tools/cipd/apps/cipd/friendly.go (right): https://chromiumcodereview.appspot.com/1358533003/diff/80001/go/src/infra/too... go/src/infra/tools/cipd/apps/cipd/friendly.go:39: if parent[len(parent)-1] == filepath.Separator { On 2015/09/30 21:03:06, nodir wrote: > according to the comment, it can be C:\ Yes, so what? In that case we at the root of the disk and can't go up, so returning "". Or did you mean that C:/.cipd/ will be ignored? Added check for that. https://chromiumcodereview.appspot.com/1358533003/diff/80001/go/src/infra/too... go/src/infra/tools/cipd/apps/cipd/friendly.go:136: // files of directories, just reads what's on disk. On 2015/09/30 21:03:06, nodir wrote: > or directories Done. https://chromiumcodereview.appspot.com/1358533003/diff/80001/go/src/infra/too... go/src/infra/tools/cipd/apps/cipd/friendly.go:178: fmt.Printf("Warning: %s.\n", msg) On 2015/09/30 21:03:06, nodir wrote: > this kind of stuff should be printed to stderr > > > if you have a lot of calls like this: > warning := log.New(os.Stderr, "Warning: ", 0) > ... > warning.Println(msg) Done. https://chromiumcodereview.appspot.com/1358533003/diff/80001/go/src/infra/too... go/src/infra/tools/cipd/apps/cipd/friendly.go:194: // only once. Use it directly then via site.client. On 2015/09/30 21:03:06, nodir wrote: > s/then/than/ or s/via/use/ Done. https://chromiumcodereview.appspot.com/1358533003/diff/80001/go/src/infra/too... go/src/infra/tools/cipd/apps/cipd/friendly.go:234: cpy := pin On 2015/09/30 21:03:05, nodir wrote: > `pin` is a copy anyway because it is stored on stack > https://play.golang.org/p/yrb3uSTbqk pin is a copy, but it gets overwritten in each iteration. &pin will always have same address. https://play.golang.org/p/wQ42b--nZF
uploaded the patchset, ptal
lgtm https://chromiumcodereview.appspot.com/1358533003/diff/80001/go/src/infra/too... File go/src/infra/tools/cipd/apps/cipd/friendly.go (right): https://chromiumcodereview.appspot.com/1358533003/diff/80001/go/src/infra/too... go/src/infra/tools/cipd/apps/cipd/friendly.go:39: if parent[len(parent)-1] == filepath.Separator { On 2015/09/30 21:41:46, Vadim Sh. wrote: > On 2015/09/30 21:03:06, nodir wrote: > > according to the comment, it can be C:\ > > Yes, so what? In that case we at the root of the disk and can't go up, so > returning "". > > Or did you mean that C:/.cipd/ will be ignored? Added check for that. err, ignore that
The CQ bit was checked by vadimsh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tandrii@google.com Link to the patchset: https://chromiumcodereview.appspot.com/1358533003/#ps100001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1358533003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1358533003/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/infra/infra/+/7bf444f470080f76364d1ed3a5a98... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
