Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2768203002:
Milo: Fix Gerrit CL reporting in buildbucket view (Closed)
|
Created:
3 years, 9 months ago by Ryan Tseng Modified:
3 years, 9 months ago 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. |
DescriptionMilo: Fix Gerrit CL reporting in buildbucket view
Turns out the properties were named wrong
BUG=687329
Review-Url: https://codereview.chromium.org/2768203002
Committed: https://github.com/luci/luci-go/commit/9c353a31df60535ce05509c32a6671ba84c3f2a5
Patch Set 1 #
Total comments: 4
Patch Set 2 : Review #
Messages
Total messages: 21 (14 generated)
The CQ bit was checked by hinoka@google.com 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...
hinoka@chromium.org changed reviewers: + hinoka@chromium.org, nodir@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
tandrii@chromium.org changed reviewers: + tandrii@chromium.org
https://codereview.chromium.org/2768203002/diff/1/milo/appengine/buildbucket/... File milo/appengine/buildbucket/common.go (right): https://codereview.chromium.org/2768203002/diff/1/milo/appengine/buildbucket/... milo/appengine/buildbucket/common.go:129: URL: fmt.Sprintf("%s/c/%d", prop.PatchGerritURL, prop.PatchIssue), wdyt about linking to specific patchset like you do in Rietveld's case. In gerrit properties, it's called 'patch_set' (because I wanted to keep 'patch_' prefix for all props set by CQ.
Also, reviewing on Rietveld is so horrible...
lgtm https://codereview.chromium.org/2768203002/diff/1/milo/appengine/buildbucket/... File milo/appengine/buildbucket/common.go (right): https://codereview.chromium.org/2768203002/diff/1/milo/appengine/buildbucket/... milo/appengine/buildbucket/common.go:129: URL: fmt.Sprintf("%s/c/%d", prop.PatchGerritURL, prop.PatchIssue), On 2017/03/23 15:32:57, tandrii(chromium) wrote: > wdyt about linking to specific patchset like you do in Rietveld's case. > In gerrit properties, it's called 'patch_set' (because I wanted to keep 'patch_' > prefix for all props set by CQ. +1 do it if GerritPatchSet != 0 https://codereview.chromium.org/2768203002/diff/1/milo/appengine/buildbucket/... File milo/appengine/buildbucket/properties.go (right): https://codereview.chromium.org/2768203002/diff/1/milo/appengine/buildbucket/... milo/appengine/buildbucket/properties.go:21: PatchSet number `json:"patchset"` // e.g. 40001 for rietveld or 1 for gerrit apparently for Gerrit it is "patch_set" with underscore. We could have Patch_Set, but I find it ugly. Why not take advantage of json field mapping and rename fields so that their names clearly indicate what they describe rather than they are mapped to? I propose the code review-related fields to have "Rietveld" and "Gerrit" prefixes. i.e. fields RietveldURL, RietveldIssue, RietveldPatchset, GerritURL, GerritChange (i am not sure why the property is called issue. issue is a rietveld term. Changes in Gerrit are called "changes", hence /c/ in path), GerritPatchSet, GerritProject, GerritRepositoryURL. After writing this comment I realized that they had "Gerrit" prefix :)
The CQ bit was checked by hinoka@google.com 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.
https://codereview.chromium.org/2768203002/diff/1/milo/appengine/buildbucket/... File milo/appengine/buildbucket/common.go (right): https://codereview.chromium.org/2768203002/diff/1/milo/appengine/buildbucket/... milo/appengine/buildbucket/common.go:129: URL: fmt.Sprintf("%s/c/%d", prop.PatchGerritURL, prop.PatchIssue), On 2017/03/24 06:35:02, nodir wrote: > On 2017/03/23 15:32:57, tandrii(chromium) wrote: > > wdyt about linking to specific patchset like you do in Rietveld's case. > > In gerrit properties, it's called 'patch_set' (because I wanted to keep > 'patch_' > > prefix for all props set by CQ. > > +1 > > do it if GerritPatchSet != 0 Done.
The CQ bit was checked by hinoka@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nodir@chromium.org Link to the patchset: https://codereview.chromium.org/2768203002/#ps20001 (title: "Review")
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": 20001, "attempt_start_ts": 1490638944679930,
"parent_rev": "26d81493a96488be4ec8b6bc00fb3129de58eb15", "commit_rev":
"9c353a31df60535ce05509c32a6671ba84c3f2a5"}
Message was sent while issue was closed.
Description was changed from ========== Milo: Fix Gerrit CL reporting in buildbucket view Turns out the properties were named wrong BUG=687329 ========== to ========== Milo: Fix Gerrit CL reporting in buildbucket view Turns out the properties were named wrong BUG=687329 Review-Url: https://codereview.chromium.org/2768203002 Committed: https://github.com/luci/luci-go/commit/9c353a31df60535ce05509c32a6671ba84c3f2a5 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://github.com/luci/luci-go/commit/9c353a31df60535ce05509c32a6671ba84c3f2a5 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
