Issue 155730043: code review 155730043: go.tools/dashboard/watcher: commit watcher rewrite (Closed)
Descriptiongo.tools/dashboard/watcher: commit watcher rewrite
Patch Set 1 #Patch Set 2 : diff -r 1781216667a64f04a9cbe33ab5484973148241bb https://code.google.com/p/go.tools #Patch Set 3 : diff -r 1781216667a64f04a9cbe33ab5484973148241bb https://code.google.com/p/go.tools #
Total comments: 1
Patch Set 4 : diff -r 1781216667a64f04a9cbe33ab5484973148241bb https://code.google.com/p/go.tools #Patch Set 5 : diff -r 1781216667a64f04a9cbe33ab5484973148241bb https://code.google.com/p/go.tools #Patch Set 6 : diff -r 1781216667a64f04a9cbe33ab5484973148241bb https://code.google.com/p/go.tools #Patch Set 7 : diff -r 1781216667a64f04a9cbe33ab5484973148241bb https://code.google.com/p/go.tools #
Total comments: 3
Patch Set 8 : diff -r 1781216667a64f04a9cbe33ab5484973148241bb https://code.google.com/p/go.tools #Patch Set 9 : diff -r 1781216667a64f04a9cbe33ab5484973148241bb https://code.google.com/p/go.tools #
Total comments: 6
Patch Set 10 : diff -r 1781216667a64f04a9cbe33ab5484973148241bb https://code.google.com/p/go.tools #Patch Set 11 : diff -r 1781216667a64f04a9cbe33ab5484973148241bb https://code.google.com/p/go.tools #Patch Set 12 : diff -r 1781216667a64f04a9cbe33ab5484973148241bb https://code.google.com/p/go.tools #Patch Set 13 : diff -r 1781216667a64f04a9cbe33ab5484973148241bb https://code.google.com/p/go.tools #Patch Set 14 : diff -r 1781216667a64f04a9cbe33ab5484973148241bb https://code.google.com/p/go.tools #Patch Set 15 : diff -r 1781216667a64f04a9cbe33ab5484973148241bb https://code.google.com/p/go.tools #
Total comments: 31
Patch Set 16 : diff -r 0732e60486ac68f13059ec7d805e6c086299bb0a https://code.google.com/p/go.tools #Patch Set 17 : diff -r 0732e60486ac68f13059ec7d805e6c086299bb0a https://code.google.com/p/go.tools #Patch Set 18 : diff -r 0732e60486ac68f13059ec7d805e6c086299bb0a https://code.google.com/p/go.tools #
Total comments: 30
Patch Set 19 : diff -r 4afcf629723f811ab20e7a0ffdc445c018a4f1eb https://code.google.com/p/go.tools #
MessagesTotal messages: 19
Hello bradfitz (cc: golang-codereviews@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go.tools
Sign in to reply to this message.
Hello bradfitz@golang.org (cc: golang-codereviews@googlegroups.com), Please take another look.
Sign in to reply to this message.
Pass by comment. Alex https://codereview.appspot.com/155730043/diff/40001/dashboard/watcher/watcher.go File dashboard/watcher/watcher.go (right): https://codereview.appspot.com/155730043/diff/40001/dashboard/watcher/watcher... dashboard/watcher/watcher.go:103: b.postNewCommits() Don't want to handle errors returned by b.postNewCommits?
Sign in to reply to this message.
Hello bradfitz@golang.org, alex.brainman@gmail.com (cc: golang-codereviews@googlegroups.com), Please take another look.
Sign in to reply to this message.
update dashboard/README too https://codereview.appspot.com/155730043/diff/160001/dashboard/watcher/watche... File dashboard/watcher/watcher.go (right): https://codereview.appspot.com/155730043/diff/160001/dashboard/watcher/watche... dashboard/watcher/watcher.go:31: dashboard = flag.String("dash", "https://build.golang.org/", "Dashboard URL") document that this must end in a slash https://codereview.appspot.com/155730043/diff/160001/dashboard/watcher/watche... dashboard/watcher/watcher.go:95: root string what is root? comment the crap out of all this. part of the reason the old builder code was painful was lack of comments. https://codereview.appspot.com/155730043/diff/160001/dashboard/watcher/watche... dashboard/watcher/watcher.go:101: func NewRepo(dir, url, path string) (*Repo, error) { there are enough parameters here to warrant some comments. I could guess at least two of these, but not three. In particular, what is path?
Sign in to reply to this message.
https://codereview.appspot.com/155730043/diff/120001/dashboard/watcher/watche... File dashboard/watcher/watcher.go (right): https://codereview.appspot.com/155730043/diff/120001/dashboard/watcher/watche... dashboard/watcher/watcher.go:76: subrepos, err := subrepoList() I would move this up a bit. Finish all init gathering before starting to do real work. https://codereview.appspot.com/155730043/diff/120001/dashboard/watcher/watche... dashboard/watcher/watcher.go:173: if next == nil { "next" could also happen to be nil if none of "c.children" is on branch "b". It shouldn't happened, but. I think you need to rethink this bit of code.
Sign in to reply to this message.
PTAL https://codereview.appspot.com/155730043/diff/160001/dashboard/watcher/watche... File dashboard/watcher/watcher.go (right): https://codereview.appspot.com/155730043/diff/160001/dashboard/watcher/watche... dashboard/watcher/watcher.go:31: dashboard = flag.String("dash", "https://build.golang.org/", "Dashboard URL") On 2014/10/03 03:55:53, bradfitz wrote: > document that this must end in a slash Done. https://codereview.appspot.com/155730043/diff/160001/dashboard/watcher/watche... dashboard/watcher/watcher.go:95: root string On 2014/10/03 03:55:53, bradfitz wrote: > what is root? comment the crap out of all this. part of the reason the old > builder code was painful was lack of comments. Done. https://codereview.appspot.com/155730043/diff/160001/dashboard/watcher/watche... dashboard/watcher/watcher.go:101: func NewRepo(dir, url, path string) (*Repo, error) { On 2014/10/03 03:55:53, bradfitz wrote: > there are enough parameters here to warrant some comments. I could guess at > least two of these, but not three. > > In particular, what is path? Done.
Sign in to reply to this message.
https://codereview.appspot.com/155730043/diff/120001/dashboard/watcher/watche... File dashboard/watcher/watcher.go (right): https://codereview.appspot.com/155730043/diff/120001/dashboard/watcher/watche... dashboard/watcher/watcher.go:173: if next == nil { On 2014/10/03 04:01:20, brainman wrote: > "next" could also happen to be nil if none of "c.children" is on branch "b". It > shouldn't happened, but. I think you need to rethink this bit of code. That's fine, actually, because postNewCommits is only responsible for posting the new commits on the specified branch. Added a comment.
Sign in to reply to this message.
Hello bradfitz@golang.org, alex.brainman@gmail.com (cc: golang-codereviews@googlegroups.com), Please take another look.
Sign in to reply to this message.
https://codereview.appspot.com/155730043/diff/280001/dashboard/watcher/watche... File dashboard/watcher/watcher.go (right): https://codereview.appspot.com/155730043/diff/280001/dashboard/watcher/watche... dashboard/watcher/watcher.go:140: return err the watcher will die after a brief central repo unavailability not good https://codereview.appspot.com/155730043/diff/280001/dashboard/watcher/watche... dashboard/watcher/watcher.go:421: args = append([]string{"log", "--template", xmlLogTemplate}, args...) how gccgo commits are updated? gccgo uses svn
Sign in to reply to this message.
https://codereview.appspot.com/155730043/diff/280001/dashboard/watcher/watche... File dashboard/watcher/watcher.go (right): https://codereview.appspot.com/155730043/diff/280001/dashboard/watcher/watche... dashboard/watcher/watcher.go:140: return err On 2014/10/04 08:26:06, dvyukov wrote: > the watcher will die after a brief central repo unavailability > not good Check the hgPull function. It tries three times before returning an error. Probably it should sleep after retrying. FWIW, it's not a big deal if the watcher exits and has to restart. I'd rather work on making it restart faster than preventing it from exiting. https://codereview.appspot.com/155730043/diff/280001/dashboard/watcher/watche... dashboard/watcher/watcher.go:421: args = append([]string{"log", "--template", xmlLogTemplate}, args...) On 2014/10/04 08:26:06, dvyukov wrote: > how gccgo commits are updated? gccgo uses svn Outside the scope of this CL. We'll make it support other VCSes later.
Sign in to reply to this message.
https://codereview.appspot.com/155730043/diff/280001/dashboard/watcher/watche... File dashboard/watcher/watcher.go (right): https://codereview.appspot.com/155730043/diff/280001/dashboard/watcher/watche... dashboard/watcher/watcher.go:140: return err On 2014/10/04 08:32:27, adg wrote: > On 2014/10/04 08:26:06, dvyukov wrote: > > the watcher will die after a brief central repo unavailability > > not good > > Check the hgPull function. It tries three times before returning an error. That won't help during repo unavailability > Probably it should sleep after retrying. > FWIW, it's not a big deal if the watcher exits and has to restart. I'd rather > work on making it restart faster than preventing it from exiting. Do we have any restart mechanism in place? It must not be a manual process. https://codereview.appspot.com/155730043/diff/280001/dashboard/watcher/watche... dashboard/watcher/watcher.go:421: args = append([]string{"log", "--template", xmlLogTemplate}, args...) On 2014/10/04 08:32:27, adg wrote: > On 2014/10/04 08:26:06, dvyukov wrote: > > how gccgo commits are updated? gccgo uses svn > > Outside the scope of this CL. We'll make it support other VCSes later. Why don't you use go.tools/vcs package?
Sign in to reply to this message.
https://codereview.appspot.com/155730043/diff/280001/dashboard/watcher/watche... File dashboard/watcher/watcher.go (right): https://codereview.appspot.com/155730043/diff/280001/dashboard/watcher/watche... dashboard/watcher/watcher.go:140: return err On 2014/10/04 08:36:00, dvyukov wrote: > On 2014/10/04 08:32:27, adg wrote: > > On 2014/10/04 08:26:06, dvyukov wrote: > > > the watcher will die after a brief central repo unavailability > > > not good > > > > Check the hgPull function. It tries three times before returning an error. > > That won't help during repo unavailability That's OK. There's no reliable way to check what's wrong when mercurial exits. I'm treating it as totally unreliable. > > Probably it should sleep after retrying. > > FWIW, it's not a big deal if the watcher exits and has to restart. I'd rather > > work on making it restart faster than preventing it from exiting. > > Do we have any restart mechanism in place? It must not be a manual process. Yes, the process will run in a docker container that is automatically restarted. https://codereview.appspot.com/155730043/diff/280001/dashboard/watcher/watche... dashboard/watcher/watcher.go:421: args = append([]string{"log", "--template", xmlLogTemplate}, args...) On 2014/10/04 08:36:00, dvyukov wrote: > On 2014/10/04 08:32:27, adg wrote: > > On 2014/10/04 08:26:06, dvyukov wrote: > > > how gccgo commits are updated? gccgo uses svn > > > > Outside the scope of this CL. We'll make it support other VCSes later. > > Why don't you use go.tools/vcs package? The brokenness of go.tools/go/vcs is what prompted me to write this from scratch. The abstractions hinder rather than help, at least in the case of the commit watcher (it was doing an "hg pull" for every "hg log"). This is at least nice and self-contained. The hg-specific code here is around 50 lines. I'm confident we can make it work with SVN pretty easily.
Sign in to reply to this message.
On 2014/10/04 08:39:23, adg wrote: > https://codereview.appspot.com/155730043/diff/280001/dashboard/watcher/watche... > File dashboard/watcher/watcher.go (right): > > https://codereview.appspot.com/155730043/diff/280001/dashboard/watcher/watche... > dashboard/watcher/watcher.go:140: return err > On 2014/10/04 08:36:00, dvyukov wrote: > > On 2014/10/04 08:32:27, adg wrote: > > > On 2014/10/04 08:26:06, dvyukov wrote: > > > > the watcher will die after a brief central repo unavailability > > > > not good > > > > > > Check the hgPull function. It tries three times before returning an error. > > > > That won't help during repo unavailability > > That's OK. There's no reliable way to check what's wrong when mercurial exits. > I'm treating it as totally unreliable. > > > > Probably it should sleep after retrying. > > > FWIW, it's not a big deal if the watcher exits and has to restart. I'd > rather > > > work on making it restart faster than preventing it from exiting. > > > > Do we have any restart mechanism in place? It must not be a manual process. > > Yes, the process will run in a docker container that is automatically restarted. ok > https://codereview.appspot.com/155730043/diff/280001/dashboard/watcher/watche... > dashboard/watcher/watcher.go:421: args = append([]string{"log", "--template", > xmlLogTemplate}, args...) > On 2014/10/04 08:36:00, dvyukov wrote: > > On 2014/10/04 08:32:27, adg wrote: > > > On 2014/10/04 08:26:06, dvyukov wrote: > > > > how gccgo commits are updated? gccgo uses svn > > > > > > Outside the scope of this CL. We'll make it support other VCSes later. > > > > Why don't you use go.tools/vcs package? > > The brokenness of go.tools/go/vcs is what prompted me to write this from > scratch. The abstractions hinder rather than help, at least in the case of the > commit watcher (it was doing an "hg pull" for every "hg log"). > > This is at least nice and self-contained. The hg-specific code here is around 50 > lines. I'm confident we can make it work with SVN pretty easily. ok
Sign in to reply to this message.
Initial comments.. Have to run. More later. https://codereview.appspot.com/155730043/diff/280001/dashboard/README File dashboard/README (right): https://codereview.appspot.com/155730043/diff/280001/dashboard/README#newcode13 dashboard/README:13: watcher/: a daemon that watches for new commits int he Go repository and typo: in the https://codereview.appspot.com/155730043/diff/280001/dashboard/README#newcode13 dashboard/README:13: watcher/: a daemon that watches for new commits int he Go repository and and repositories plural? https://codereview.appspot.com/155730043/diff/280001/dashboard/watcher/watche... File dashboard/watcher/watcher.go (right): https://codereview.appspot.com/155730043/diff/280001/dashboard/watcher/watche... dashboard/watcher/watcher.go:30: local = flag.String("local", "", "Local repo (for testing only)") rearrange this to be after the real flags, with a blank line separating them? https://codereview.appspot.com/155730043/diff/280001/dashboard/watcher/watche... dashboard/watcher/watcher.go:70: errc := make(chan error) wait, you use this channel to do 1+N sends, but only do 1 read. so you're missing synchronization, no? https://codereview.appspot.com/155730043/diff/280001/dashboard/watcher/watche... dashboard/watcher/watcher.go:87: r, err := NewRepo(dir, "https://"+p, p) wait, what form is p in? document this. confusing. https://codereview.appspot.com/155730043/diff/280001/dashboard/watcher/watche... dashboard/watcher/watcher.go:102: path string // blank for main repo else what? https://codereview.appspot.com/155730043/diff/280001/dashboard/watcher/watche... dashboard/watcher/watcher.go:103: commits map[string]*Commit what is key? N digit (what N?) lower hex? https://codereview.appspot.com/155730043/diff/280001/dashboard/watcher/watche... dashboard/watcher/watcher.go:104: branches map[string]*Branch give an example of a key. e.g. "release-go1.3" or some full URL? https://codereview.appspot.com/155730043/diff/280001/dashboard/watcher/watche... dashboard/watcher/watcher.go:164: func (r *Repo) postNewCommits(b *Branch) error { should this be a method on the Branch? *shrug* https://codereview.appspot.com/155730043/diff/280001/dashboard/watcher/watche... dashboard/watcher/watcher.go:220: r.path, I'm uncomfortable with this many position strings in a row. Even though the type is up above, I'm afraid of them falling out of sync while being rearranged. I'd add struct field names here. https://codereview.appspot.com/155730043/diff/280001/dashboard/watcher/watche... dashboard/watcher/watcher.go:544: func subrepoList() ([]string, error) { document the return type example values https://codereview.appspot.com/155730043/diff/280001/dashboard/watcher/watche... dashboard/watcher/watcher.go:573: return err include out? fmt.Sprintf("Error running hg help templates: %v, %s", err, out) Could help diagnose bad Docker containers with hg messed up.
Sign in to reply to this message.
PTAL https://codereview.appspot.com/155730043/diff/280001/dashboard/watcher/watche... File dashboard/watcher/watcher.go (right): https://codereview.appspot.com/155730043/diff/280001/dashboard/watcher/watche... dashboard/watcher/watcher.go:30: local = flag.String("local", "", "Local repo (for testing only)") On 2014/10/04 19:35:05, bradfitz wrote: > rearrange this to be after the real flags, with a blank line separating them? Removed the flag. https://codereview.appspot.com/155730043/diff/280001/dashboard/watcher/watche... dashboard/watcher/watcher.go:70: errc := make(chan error) On 2014/10/04 19:35:05, bradfitz wrote: > wait, you use this channel to do 1+N sends, but only do 1 read. so you're > missing synchronization, no? The program exits after the run function returns, anyway, so I don't worry about other blocked goroutines. The only reason I'm using a run function is to be able to return errors instead of duplicating the os.Exit(1) each time (and also to use defer to remove the temp directory). https://codereview.appspot.com/155730043/diff/280001/dashboard/watcher/watche... dashboard/watcher/watcher.go:87: r, err := NewRepo(dir, "https://"+p, p) On 2014/10/04 19:35:05, bradfitz wrote: > wait, what form is p in? document this. confusing. Done. https://codereview.appspot.com/155730043/diff/280001/dashboard/watcher/watche... dashboard/watcher/watcher.go:102: path string // blank for main repo On 2014/10/04 19:35:05, bradfitz wrote: > else what? Done. https://codereview.appspot.com/155730043/diff/280001/dashboard/watcher/watche... dashboard/watcher/watcher.go:103: commits map[string]*Commit On 2014/10/04 19:35:05, bradfitz wrote: > what is key? N digit (what N?) lower hex? Done. https://codereview.appspot.com/155730043/diff/280001/dashboard/watcher/watche... dashboard/watcher/watcher.go:104: branches map[string]*Branch On 2014/10/04 19:35:05, bradfitz wrote: > give an example of a key. e.g. "release-go1.3" or some full URL? Done. https://codereview.appspot.com/155730043/diff/280001/dashboard/watcher/watche... dashboard/watcher/watcher.go:164: func (r *Repo) postNewCommits(b *Branch) error { On 2014/10/04 19:35:05, bradfitz wrote: > should this be a method on the Branch? *shrug* It was, but then I needed to use the *Repo's postCommit method, so it makes more sense for this to be a method on *Repo too. https://codereview.appspot.com/155730043/diff/280001/dashboard/watcher/watche... dashboard/watcher/watcher.go:220: r.path, On 2014/10/04 19:35:05, bradfitz wrote: > I'm uncomfortable with this many position strings in a row. Even though the type > is up above, I'm afraid of them falling out of sync while being rearranged. I'd > add struct field names here. Done. https://codereview.appspot.com/155730043/diff/280001/dashboard/watcher/watche... dashboard/watcher/watcher.go:304: // Find all commits from known head. I significantly simplified this function, by removing the code that detects new branches. In the next CL I will figure out how to detect new branches. https://codereview.appspot.com/155730043/diff/280001/dashboard/watcher/watche... dashboard/watcher/watcher.go:544: func subrepoList() ([]string, error) { On 2014/10/04 19:35:05, bradfitz wrote: > document the return type example values Done. https://codereview.appspot.com/155730043/diff/280001/dashboard/watcher/watche... dashboard/watcher/watcher.go:573: return err On 2014/10/04 19:35:05, bradfitz wrote: > include out? fmt.Sprintf("Error running hg help templates: %v, %s", err, out) > > Could help diagnose bad Docker containers with hg messed up. Done.
Sign in to reply to this message.
LGTM But see comments first. https://codereview.appspot.com/155730043/diff/340001/dashboard/README File dashboard/README (right): https://codereview.appspot.com/155730043/diff/340001/dashboard/README#newcode13 dashboard/README:13: watcher/: a daemon that watches for new commits int he Go repository and typo: in the https://codereview.appspot.com/155730043/diff/340001/dashboard/watcher/watche... File dashboard/watcher/watcher.go (right): https://codereview.appspot.com/155730043/diff/340001/dashboard/watcher/watche... dashboard/watcher/watcher.go:30: dashboard = flag.String("dash", "https://build.golang.org/", "Dashboard URL (must end in /)") might as well also check this and log.Fatalf if it doesn't end in slash https://codereview.appspot.com/155730043/diff/340001/dashboard/watcher/watche... dashboard/watcher/watcher.go:40: // The first main repo commit on the dashboard; ignore commits before this. clarify that this is for the main Go hg repo only? (i.e. not gccgo svn) https://codereview.appspot.com/155730043/diff/340001/dashboard/watcher/watche... dashboard/watcher/watcher.go:114: r.root = filepath.Join(dir, filepath.Base(path)) why is this r.root but you set path above? I'd do both in the composite literal with one field per line https://codereview.appspot.com/155730043/diff/340001/dashboard/watcher/watche... dashboard/watcher/watcher.go:115: cmd := exec.Command("hg", "clone", url, r.root) does hg clone do nothing if the path is already there? Or are we assuming that this directory needs to be non-existent already? https://codereview.appspot.com/155730043/diff/340001/dashboard/watcher/watche... dashboard/watcher/watcher.go:115: cmd := exec.Command("hg", "clone", url, r.root) hg clone can take a long time. can you put some log statements before & after this runs, so we see its progress? https://codereview.appspot.com/155730043/diff/340001/dashboard/watcher/watche... dashboard/watcher/watcher.go:292: return c.Hash != "b59f4ff1b51094314f735a4d57a2b8f06cfadf15" && this reads weird for me. I'd prefer more explicit, like: if c.Hash == "..." || c.Hash == "..." { // these are old, so we ignore them: return false } // by default, all branches are valid. return true https://codereview.appspot.com/155730043/diff/340001/dashboard/watcher/watche... dashboard/watcher/watcher.go:368: u := *dashboard + "commit?hash=" + c.Hash + "&packagePath=" + r.path r.path should be URL-escaped? https://codereview.appspot.com/155730043/diff/340001/dashboard/watcher/watche... dashboard/watcher/watcher.go:381: if resp.Error == "" { this is kinda gross, treating all errors as 404. I don't like that aspect of the dashboard's HTTP interface. I'd use 404 vs 500 etc. But at least add a comment here about the interface and saying why it's okay (if it is) that a server error (datastore being down, or 403 or quota limits) is a false negative here. https://codereview.appspot.com/155730043/diff/340001/dashboard/watcher/watche... dashboard/watcher/watcher.go:385: return nil, nil document in func comment what nil, nil means? https://codereview.appspot.com/155730043/diff/340001/dashboard/watcher/watche... dashboard/watcher/watcher.go:406: err = xml.Unmarshal([]byte("<Top>"+string(out)+"</Top>"), &logStruct) or xml.NewDecoder(io.MultiReader( strings.NewReader("<Top>"), bytes.NewReader(out), strings.NewReader("</Top>")).Decode(&logStruct) https://codereview.appspot.com/155730043/diff/340001/dashboard/watcher/watche... dashboard/watcher/watcher.go:426: continue sleep a bit before the continue? https://codereview.appspot.com/155730043/diff/340001/dashboard/watcher/watche... dashboard/watcher/watcher.go:449: Desc string what form is this in? you seem to assume something of its form on line 460 https://codereview.appspot.com/155730043/diff/340001/dashboard/watcher/watche... dashboard/watcher/watcher.go:463: // NeedsBenchmarking determines whether the Commit needs benchmarking. s/determines/reports/ ? https://codereview.appspot.com/155730043/diff/340001/dashboard/watcher/watche... dashboard/watcher/watcher.go:543: // checkHgVersion checks whether the instlaled typo and incomplete sentence.
Sign in to reply to this message.
https://codereview.appspot.com/155730043/diff/340001/dashboard/README File dashboard/README (right): https://codereview.appspot.com/155730043/diff/340001/dashboard/README#newcode13 dashboard/README:13: watcher/: a daemon that watches for new commits int he Go repository and On 2014/10/06 04:49:37, bradfitz wrote: > typo: in the Done. https://codereview.appspot.com/155730043/diff/340001/dashboard/watcher/watche... File dashboard/watcher/watcher.go (right): https://codereview.appspot.com/155730043/diff/340001/dashboard/watcher/watche... dashboard/watcher/watcher.go:30: dashboard = flag.String("dash", "https://build.golang.org/", "Dashboard URL (must end in /)") On 2014/10/06 04:49:38, bradfitz wrote: > might as well also check this and log.Fatalf if it doesn't end in slash Done. https://codereview.appspot.com/155730043/diff/340001/dashboard/watcher/watche... dashboard/watcher/watcher.go:40: // The first main repo commit on the dashboard; ignore commits before this. On 2014/10/06 04:49:38, bradfitz wrote: > clarify that this is for the main Go hg repo only? (i.e. not gccgo svn) Done. https://codereview.appspot.com/155730043/diff/340001/dashboard/watcher/watche... dashboard/watcher/watcher.go:114: r.root = filepath.Join(dir, filepath.Base(path)) On 2014/10/06 04:49:37, bradfitz wrote: > why is this r.root but you set path above? I'd do both in the composite literal > with one field per line Done. https://codereview.appspot.com/155730043/diff/340001/dashboard/watcher/watche... dashboard/watcher/watcher.go:115: cmd := exec.Command("hg", "clone", url, r.root) On 2014/10/06 04:49:38, bradfitz wrote: > hg clone can take a long time. can you put some log statements before & after > this runs, so we see its progress? Done. https://codereview.appspot.com/155730043/diff/340001/dashboard/watcher/watche... dashboard/watcher/watcher.go:115: cmd := exec.Command("hg", "clone", url, r.root) On 2014/10/06 04:49:38, bradfitz wrote: > does hg clone do nothing if the path is already there? Or are we assuming that > this directory needs to be non-existent already? I removed the flag that lets you specify an existing directory (for now), so this just assumes the directory doesn't exist. https://codereview.appspot.com/155730043/diff/340001/dashboard/watcher/watche... dashboard/watcher/watcher.go:292: return c.Hash != "b59f4ff1b51094314f735a4d57a2b8f06cfadf15" && On 2014/10/06 04:49:38, bradfitz wrote: > this reads weird for me. I'd prefer more explicit, like: > > if c.Hash == "..." || c.Hash == "..." { > // these are old, so we ignore them: > return false > } > > // by default, all branches are valid. > return true Done. https://codereview.appspot.com/155730043/diff/340001/dashboard/watcher/watche... dashboard/watcher/watcher.go:368: u := *dashboard + "commit?hash=" + c.Hash + "&packagePath=" + r.path On 2014/10/06 04:49:37, bradfitz wrote: > r.path should be URL-escaped? used url.Values and Encode https://codereview.appspot.com/155730043/diff/340001/dashboard/watcher/watche... dashboard/watcher/watcher.go:381: if resp.Error == "" { On 2014/10/06 04:49:38, bradfitz wrote: > this is kinda gross, treating all errors as 404. I don't like that aspect of the > dashboard's HTTP interface. I'd use 404 vs 500 etc. > > But at least add a comment here about the interface and saying why it's okay (if > it is) that a server error (datastore being down, or 403 or quota limits) is a > false negative here. Updated the dashboard to return a specific error string. See: https://codereview.appspot.com/153070043 https://codereview.appspot.com/155730043/diff/340001/dashboard/watcher/watche... dashboard/watcher/watcher.go:385: return nil, nil On 2014/10/06 04:49:38, bradfitz wrote: > document in func comment what nil, nil means? Done. https://codereview.appspot.com/155730043/diff/340001/dashboard/watcher/watche... dashboard/watcher/watcher.go:406: err = xml.Unmarshal([]byte("<Top>"+string(out)+"</Top>"), &logStruct) On 2014/10/06 04:49:37, bradfitz wrote: > or xml.NewDecoder(io.MultiReader( > strings.NewReader("<Top>"), > bytes.NewReader(out), > strings.NewReader("</Top>")).Decode(&logStruct) Done. https://codereview.appspot.com/155730043/diff/340001/dashboard/watcher/watche... dashboard/watcher/watcher.go:426: continue On 2014/10/06 04:49:38, bradfitz wrote: > sleep a bit before the continue? Done. https://codereview.appspot.com/155730043/diff/340001/dashboard/watcher/watche... dashboard/watcher/watcher.go:449: Desc string On 2014/10/06 04:49:38, bradfitz wrote: > what form is this in? you seem to assume something of its form on line 460 Done. https://codereview.appspot.com/155730043/diff/340001/dashboard/watcher/watche... dashboard/watcher/watcher.go:463: // NeedsBenchmarking determines whether the Commit needs benchmarking. On 2014/10/06 04:49:38, bradfitz wrote: > s/determines/reports/ ? Done. https://codereview.appspot.com/155730043/diff/340001/dashboard/watcher/watche... dashboard/watcher/watcher.go:543: // checkHgVersion checks whether the instlaled On 2014/10/06 04:49:38, bradfitz wrote: > typo and incomplete sentence. Done.
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=8ef62ff7f1d5&repo=tools *** go.tools/dashboard/watcher: commit watcher rewrite LGTM=bradfitz R=bradfitz, alex.brainman, dvyukov CC=golang-codereviews https://codereview.appspot.com/155730043
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
