Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2983513002:
gitiles.Log: implement paging. (Closed)
|
Created:
3 years, 5 months ago by tandrii(chromium) Modified:
3 years, 5 months ago Reviewers:
Vadim Sh. CC:
chromium-reviews, infra-reviews+luci-go_chromium.org, maruel+w_chromium.org, tandrii+luci-go_chromium.org Target Ref:
refs/heads/master Project:
luci-go Visibility:
Public. |
DescriptionImprove gitiles.Log.
* implement paging
* add docs
* add tests
BUG=646067
Review-Url: https://codereview.chromium.org/2983513002
Committed: https://github.com/luci/luci-go/commit/c4abe79ec19a51f9bcb02e4aaaee09f9325a3cbf
Patch Set 1 #Patch Set 2 : gitiles.Log: implement paging. #Patch Set 3 : gitiles api: impelement paging for Log command. #Patch Set 4 : git squash commit for G-300. #Patch Set 5 : more #Patch Set 6 : Better doc. #
Total comments: 19
Patch Set 7 : review #
Total comments: 9
Patch Set 8 : with client #
Total comments: 2
Patch Set 9 : more review #Patch Set 10 : fix milo #
Total comments: 2
Patch Set 11 : nit #Patch Set 12 : fix #
Dependent Patchsets: Messages
Total messages: 63 (49 generated)
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== gitiles.Log: implement paging. BUG=646067 ========== to ========== gitiles.Log: implement paging + add tests. BUG=646067 ==========
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== gitiles.Log: implement paging + add tests. BUG=646067 ========== to ========== Improve gitiles.Log. * implement paging * add docs * add tests BUG=646067 ==========
tandrii@chromium.org changed reviewers: + vadimsh@chromium.org
ptal
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
nice doc comment https://codereview.chromium.org/2983513002/diff/100001/common/api/gitiles/git... File common/api/gitiles/gitiles.go (right): https://codereview.chromium.org/2983513002/diff/100001/common/api/gitiles/git... common/api/gitiles/gitiles.go:62: // error is returned is validation fails. if validation fails https://codereview.chromium.org/2983513002/diff/100001/common/api/gitiles/git... common/api/gitiles/gitiles.go:71: if !strings.HasSuffix(u.Host, ".googlesource.com") { this reminds me of general uselessness of luci as open source project :) https://codereview.chromium.org/2983513002/diff/100001/common/api/gitiles/git... common/api/gitiles/gitiles.go:127: logURL = fmt.Sprintf("%s/+log/%s?format=JSON", logURL, treeish) url-encode treeish to be safe (assuming gitiles is ok with that) https://codereview.chromium.org/2983513002/diff/100001/common/api/gitiles/git... common/api/gitiles/gitiles.go:128: t, err := auth.GetRPCTransport(c, auth.NoAuth) strictly speaking, this library will be much more usable (and will deserve the right to be in 'common', which is supposedly usable from both client and server environments), if http.Client can be passed from outside: type Client struct { Client *http.Client } func (c *Client) Log(context.Context, ....) (....) { ... } Then we can call gitiles from client-side tools. And on server side callers can decide for themselves what authentication to use. https://codereview.chromium.org/2983513002/diff/100001/common/api/gitiles/git... common/api/gitiles/gitiles.go:140: return result, nil do we want to trim this to the limit? It may be surprising for the caller to receive more than 'limit'. https://codereview.chromium.org/2983513002/diff/100001/common/api/gitiles/git... common/api/gitiles/gitiles.go:160: r, err := client.Get(URL) please use https://godoc.org/golang.org/x/net/context/ctxhttp#Get https://codereview.chromium.org/2983513002/diff/100001/common/api/gitiles/git... common/api/gitiles/gitiles.go:162: return err we should probably retry transient errors (like HTTP 500), or at least annotate them, so that callers know when its safe to retry. https://codereview.chromium.org/2983513002/diff/100001/common/api/gitiles/git... common/api/gitiles/gitiles.go:170: if c, err := r.Body.Read(trash); err != nil || c != 4 { please assert that 'trash' is indeed ")]}'"
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ptal again :) https://codereview.chromium.org/2983513002/diff/100001/common/api/gitiles/git... File common/api/gitiles/gitiles.go (right): https://codereview.chromium.org/2983513002/diff/100001/common/api/gitiles/git... common/api/gitiles/gitiles.go:62: // error is returned is validation fails. On 2017/07/19 at 17:09:24, Vadim Sh. wrote: > if validation fails Done. https://codereview.chromium.org/2983513002/diff/100001/common/api/gitiles/git... common/api/gitiles/gitiles.go:71: if !strings.HasSuffix(u.Host, ".googlesource.com") { On 2017/07/19 at 17:09:24, Vadim Sh. wrote: > this reminds me of general uselessness of luci as open source project :) yeah, i had the same thought :( https://codereview.chromium.org/2983513002/diff/100001/common/api/gitiles/git... common/api/gitiles/gitiles.go:127: logURL = fmt.Sprintf("%s/+log/%s?format=JSON", logURL, treeish) On 2017/07/19 at 17:09:24, Vadim Sh. wrote: > url-encode treeish to be safe (assuming gitiles is ok with that) tested gitiles, they seem OK. https://codereview.chromium.org/2983513002/diff/100001/common/api/gitiles/git... common/api/gitiles/gitiles.go:128: t, err := auth.GetRPCTransport(c, auth.NoAuth) On 2017/07/19 at 17:09:24, Vadim Sh. wrote: > strictly speaking, this library will be much more usable (and will deserve the right to be in 'common', which is supposedly usable from both client and server environments), if http.Client can be passed from outside: > > type Client struct { > Client *http.Client > } > > func (c *Client) Log(context.Context, ....) (....) { > ... > } > > Then we can call gitiles from client-side tools. And on server side callers can decide for themselves what authentication to use. Yay, I thought of this too bcz it'd have made testing so much easier + maybe keepalive can shelve some latency. Mind if I do this change as a follow up CL? https://codereview.chromium.org/2983513002/diff/100001/common/api/gitiles/git... common/api/gitiles/gitiles.go:140: return result, nil On 2017/07/19 at 17:09:24, Vadim Sh. wrote: > do we want to trim this to the limit? It may be surprising for the caller to receive more than 'limit'. trimmed. https://codereview.chromium.org/2983513002/diff/100001/common/api/gitiles/git... common/api/gitiles/gitiles.go:160: r, err := client.Get(URL) On 2017/07/19 at 17:09:24, Vadim Sh. wrote: > please use https://godoc.org/golang.org/x/net/context/ctxhttp#Get Good point. done. https://codereview.chromium.org/2983513002/diff/100001/common/api/gitiles/git... common/api/gitiles/gitiles.go:162: return err On 2017/07/19 at 17:09:24, Vadim Sh. wrote: > we should probably retry transient errors (like HTTP 500), or at least annotate them, so that callers know when its safe to retry. annotated + todo. https://codereview.chromium.org/2983513002/diff/100001/common/api/gitiles/git... common/api/gitiles/gitiles.go:170: if c, err := r.Body.Read(trash); err != nil || c != 4 { On 2017/07/19 at 17:09:24, Vadim Sh. wrote: > please assert that 'trash' is indeed ")]}'" Done. https://codereview.chromium.org/2983513002/diff/100001/common/api/gitiles/git... File common/api/gitiles/gitiles_test.go (right): https://codereview.chromium.org/2983513002/diff/100001/common/api/gitiles/git... common/api/gitiles/gitiles_test.go:165: // TODO(<REVIEWER>): any easier/better way to do this? I couldn't find anything inside luci-go... any comment for this thing?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2983513002/diff/100001/common/api/gitiles/git... File common/api/gitiles/gitiles.go (right): https://codereview.chromium.org/2983513002/diff/100001/common/api/gitiles/git... common/api/gitiles/gitiles.go:128: t, err := auth.GetRPCTransport(c, auth.NoAuth) On 2017/07/19 20:12:31, tandrii(chromium) wrote: > On 2017/07/19 at 17:09:24, Vadim Sh. wrote: > > strictly speaking, this library will be much more usable (and will deserve the > right to be in 'common', which is supposedly usable from both client and server > environments), if http.Client can be passed from outside: > > > > type Client struct { > > Client *http.Client > > } > > > > func (c *Client) Log(context.Context, ....) (....) { > > ... > > } > > > > Then we can call gitiles from client-side tools. And on server side callers > can decide for themselves what authentication to use. > > Yay, I thought of this too bcz it'd have made testing so much easier + maybe > keepalive can shelve some latency. Mind if I do this change as a follow up CL? ok https://codereview.chromium.org/2983513002/diff/120001/common/api/gitiles/git... File common/api/gitiles/gitiles.go (right): https://codereview.chromium.org/2983513002/diff/120001/common/api/gitiles/git... common/api/gitiles/gitiles.go:147: result = result[0:limit] nit: just result[:limit] https://codereview.chromium.org/2983513002/diff/120001/common/api/gitiles/git... common/api/gitiles/gitiles.go:171: return err this is always transient error too non-transient errors are HTTP replies with status code <500, not getting a reply at all is thus transient error (in practice on GAE it is RCP timeouts) (on non-GAE it is DNS errors and dial timeouts) https://codereview.chromium.org/2983513002/diff/120001/common/api/gitiles/git... common/api/gitiles/gitiles.go:185: return errors.Annotate(err, "unexpected response from Gitiles").Err() this will return nil if trash happened to be something else (becaue err is nil in this case and annotating nil returns nil). https://codereview.chromium.org/2983513002/diff/120001/common/api/gitiles/git... File common/api/gitiles/gitiles_test.go (right): https://codereview.chromium.org/2983513002/diff/120001/common/api/gitiles/git... common/api/gitiles/gitiles_test.go:45: f("https://a.googlesource.com/") check also how it reacts to #fragments and unspecified protocols ("a.googlesource.com/a/repo") https://codereview.chromium.org/2983513002/diff/120001/common/api/gitiles/git... common/api/gitiles/gitiles_test.go:176: // TODO(<REVIEWER>): any easier/better way to do this? I couldn't find anything inside luci-go... client := Client{ mockedGitiles : srv.URL } func (c *Client) Log(...) ... { ... if c.mockedGitiles != "" { logURL = c.mockedGitiles } logURL = fmt.Sprintf("%s/+log/%s?format=JSON", logURL, url.PathEscape(treeish)) ... } And then just use regular test server without TLS stuff.
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL https://codereview.chromium.org/2983513002/diff/100001/common/api/gitiles/git... File common/api/gitiles/gitiles.go (right): https://codereview.chromium.org/2983513002/diff/100001/common/api/gitiles/git... common/api/gitiles/gitiles.go:128: t, err := auth.GetRPCTransport(c, auth.NoAuth) On 2017/07/19 23:02:30, Vadim Sh. wrote: > On 2017/07/19 20:12:31, tandrii(chromium) wrote: > > On 2017/07/19 at 17:09:24, Vadim Sh. wrote: > > > strictly speaking, this library will be much more usable (and will deserve > the > > right to be in 'common', which is supposedly usable from both client and > server > > environments), if http.Client can be passed from outside: > > > > > > type Client struct { > > > Client *http.Client > > > } > > > > > > func (c *Client) Log(context.Context, ....) (....) { > > > ... > > > } > > > > > > Then we can call gitiles from client-side tools. And on server side callers > > can decide for themselves what authentication to use. > > > > Yay, I thought of this too bcz it'd have made testing so much easier + maybe > > keepalive can shelve some latency. Mind if I do this change as a follow up CL? > > ok Actually, Done here. https://codereview.chromium.org/2983513002/diff/120001/common/api/gitiles/git... File common/api/gitiles/gitiles.go (right): https://codereview.chromium.org/2983513002/diff/120001/common/api/gitiles/git... common/api/gitiles/gitiles.go:147: result = result[0:limit] On 2017/07/19 23:02:30, Vadim Sh. wrote: > nit: just result[:limit] Done. https://codereview.chromium.org/2983513002/diff/120001/common/api/gitiles/git... common/api/gitiles/gitiles.go:185: return errors.Annotate(err, "unexpected response from Gitiles").Err() On 2017/07/19 23:02:30, Vadim Sh. wrote: > this will return nil if trash happened to be something else (becaue err is nil > in this case and annotating nil returns nil). Oops, fixed. https://codereview.chromium.org/2983513002/diff/120001/common/api/gitiles/git... File common/api/gitiles/gitiles_test.go (right): https://codereview.chromium.org/2983513002/diff/120001/common/api/gitiles/git... common/api/gitiles/gitiles_test.go:45: f("https://a.googlesource.com/") On 2017/07/19 23:02:31, Vadim Sh. wrote: > check also how it reacts to #fragments and unspecified protocols > ("a.googlesource.com/a/repo") Done. https://codereview.chromium.org/2983513002/diff/120001/common/api/gitiles/git... common/api/gitiles/gitiles_test.go:176: // TODO(<REVIEWER>): any easier/better way to do this? I couldn't find anything inside luci-go... On 2017/07/19 23:02:31, Vadim Sh. wrote: > client := Client{ > mockedGitiles : srv.URL > } > > func (c *Client) Log(...) ... { > ... > if c.mockedGitiles != "" { > logURL = c.mockedGitiles > } > logURL = fmt.Sprintf("%s/+log/%s?format=JSON", logURL, > url.PathEscape(treeish)) > ... > } > > And then just use regular test server without TLS stuff. Thanks, I've added RepoURL into gitiles client.
https://codereview.chromium.org/2983513002/diff/140001/common/api/gitiles/git... File common/api/gitiles/gitiles.go (right): https://codereview.chromium.org/2983513002/diff/140001/common/api/gitiles/git... common/api/gitiles/gitiles.go:95: func New(c *http.Client, repoURL string) (*Client, error) { I'm not a fan of such ambiguous constructors. How do I know I should use New instead of initializing Client{} struct manually? All fields are public and there are no "complex" fields - one would assume Client{} is usable without New. Also, binding Client to a particular repo is unnecessarily strict. I can easily imagine situations where we need to fetch stuff from multiple repos at the same time. Constructing Client's for each of them may feel heavy. IMHO, it'll be cleaner to keep Client constructor-less and move repoURL as argument to Log.
On 2017/07/20 16:48:13, Vadim Sh. wrote: > https://codereview.chromium.org/2983513002/diff/140001/common/api/gitiles/git... > File common/api/gitiles/gitiles.go (right): > > https://codereview.chromium.org/2983513002/diff/140001/common/api/gitiles/git... > common/api/gitiles/gitiles.go:95: func New(c *http.Client, repoURL string) > (*Client, error) { > I'm not a fan of such ambiguous constructors. How do I know I should use New > instead of initializing Client{} struct manually? All fields are public and > there are no "complex" fields - one would assume Client{} is usable without New. That's fair. I was considering of making Client an interface s.t. I can mock gitiles in scheduler tests, but ended up short of that yet with ambiguous New and &Clinet{}. WDYT? > > Also, binding Client to a particular repo is unnecessarily strict. I can easily > imagine situations where we need to fetch stuff from multiple repos at the same > time. Constructing Client's for each of them may feel heavy. > > IMHO, it'll be cleaner to keep Client constructor-less and move repoURL as > argument to Log. Mkay, moving repoURL back to args.
On 2017/07/20 16:57:21, tandrii(chromium) wrote: > On 2017/07/20 16:48:13, Vadim Sh. wrote: > > > https://codereview.chromium.org/2983513002/diff/140001/common/api/gitiles/git... > > File common/api/gitiles/gitiles.go (right): > > > > > https://codereview.chromium.org/2983513002/diff/140001/common/api/gitiles/git... > > common/api/gitiles/gitiles.go:95: func New(c *http.Client, repoURL string) > > (*Client, error) { > > I'm not a fan of such ambiguous constructors. How do I know I should use New > > instead of initializing Client{} struct manually? All fields are public and > > there are no "complex" fields - one would assume Client{} is usable without > New. > > That's fair. I was considering of making Client an interface s.t. I can mock > gitiles in scheduler tests, but ended up short of that yet with ambiguous New > and &Clinet{}. > WDYT? Adding interfaces in implementation part of the package is usually an anti-pattern. If scheduler wants mocks, _it_ should declare an interface it consumes (that matches subset of gitiles.Client{}), and then provide mock implementation in the unit tests: http://idiomaticgo.com/post/best-practice/accept-interfaces-return-structs/ Using interfaces in the package implementation is justifiable if the package itself provides multiple realization of this interface.
On 2017/07/20 17:05:45, Vadim Sh. wrote: > On 2017/07/20 16:57:21, tandrii(chromium) wrote: > > On 2017/07/20 16:48:13, Vadim Sh. wrote: > > > > > > https://codereview.chromium.org/2983513002/diff/140001/common/api/gitiles/git... > > > File common/api/gitiles/gitiles.go (right): > > > > > > > > > https://codereview.chromium.org/2983513002/diff/140001/common/api/gitiles/git... > > > common/api/gitiles/gitiles.go:95: func New(c *http.Client, repoURL string) > > > (*Client, error) { > > > I'm not a fan of such ambiguous constructors. How do I know I should use New > > > instead of initializing Client{} struct manually? All fields are public and > > > there are no "complex" fields - one would assume Client{} is usable without > > New. > > > > That's fair. I was considering of making Client an interface s.t. I can mock > > gitiles in scheduler tests, but ended up short of that yet with ambiguous New > > and &Clinet{}. > > WDYT? > > Adding interfaces in implementation part of the package is usually an > anti-pattern. If scheduler wants mocks, _it_ should declare an interface it > consumes (that matches subset of gitiles.Client{}), and then provide mock > implementation in the unit tests: > http://idiomaticgo.com/post/best-practice/accept-interfaces-return-structs/ > > Using interfaces in the package implementation is justifiable if the package > itself provides multiple realization of this interface. Read it, thanks for ref! And done.
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
https://codereview.chromium.org/2983513002/diff/140001/common/api/gitiles/git... File common/api/gitiles/gitiles.go (right): https://codereview.chromium.org/2983513002/diff/140001/common/api/gitiles/git... common/api/gitiles/gitiles.go:95: func New(c *http.Client, repoURL string) (*Client, error) { On 2017/07/20 16:48:12, Vadim Sh. wrote: > I'm not a fan of such ambiguous constructors. How do I know I should use New > instead of initializing Client{} struct manually? All fields are public and > there are no "complex" fields - one would assume Client{} is usable without New. > > Also, binding Client to a particular repo is unnecessarily strict. I can easily > imagine situations where we need to fetch stuff from multiple repos at the same > time. Constructing Client's for each of them may feel heavy. > > IMHO, it'll be cleaner to keep Client constructor-less and move repoURL as > argument to Log. Done!
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with one more nit https://codereview.chromium.org/2983513002/diff/180001/common/api/gitiles/git... File common/api/gitiles/gitiles.go (right): https://codereview.chromium.org/2983513002/diff/180001/common/api/gitiles/git... common/api/gitiles/gitiles.go:94: func New(c *http.Client) *Client { just drop this function, it's totally normal in go to initialize objects directly c := gitiles.Client{ Client: http.DefaultClient, }
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2983513002/diff/180001/common/api/gitiles/git... File common/api/gitiles/gitiles.go (right): https://codereview.chromium.org/2983513002/diff/180001/common/api/gitiles/git... common/api/gitiles/gitiles.go:94: func New(c *http.Client) *Client { On 2017/07/20 21:04:47, Vadim Sh. wrote: > just drop this function, it's totally normal in go to initialize objects > directly > > c := gitiles.Client{ > Client: http.DefaultClient, > } Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Luci-go Linux Trusty 32-on-64 Tester on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/377aa8b4e0e11410)
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tandrii@chromium.org
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/2983513002/#ps220001 (title: "fix")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1500625052246440,
"parent_rev": "8ffe319b319921ef4e757c2a6c8b7811e4913c10", "commit_rev":
"c4abe79ec19a51f9bcb02e4aaaee09f9325a3cbf"}
Message was sent while issue was closed.
Description was changed from ========== Improve gitiles.Log. * implement paging * add docs * add tests BUG=646067 ========== to ========== Improve gitiles.Log. * implement paging * add docs * add tests BUG=646067 Review-Url: https://codereview.chromium.org/2983513002 Committed: https://github.com/luci/luci-go/commit/c4abe79ec19a51f9bcb02e4aaaee09f9325a3cbf ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://github.com/luci/luci-go/commit/c4abe79ec19a51f9bcb02e4aaaee09f9325a3cbf |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
