Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1849283002:
AutoRolls include Chromium bug links (Closed)
|
Created:
4 years, 8 months ago by borenet2 Modified:
4 years, 8 months ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/buildbot@master Target Ref:
refs/heads/master Project:
skiabuildbot Visibility:
Public. |
DescriptionAutoRolls include Chromium bug links
BUG=598773
Committed: https://skia.googlesource.com/buildbot/+/2fb6667166f94531c6fd0df13570f5d1b650922a
Patch Set 1 #Patch Set 2 : Move bug finding into repo manager #Patch Set 3 : ... #
Total comments: 2
Messages
Total messages: 13 (3 generated)
borenet@google.com changed reviewers: + borenet@google.com, rmistry@google.com
Unfortunately, I haven't been able to test whether this actually adds the right bugs. It's difficult to test since it would require rolling a dummy repo into Chrome.
https://codereview.chromium.org/1849283002/diff/40001/go/util/util.go File go/util/util.go (right): https://codereview.chromium.org/1849283002/diff/40001/go/util/util.go#newcode636 go/util/util.go:636: return rv Why return a dictionary of projects to bugs? It would be simpler and still correct to just parse out all strings delimited by ',' and return an array of bugs which might contain contents like ["123", "chromium:123", "skia:123"]. You can then blindly specify them in --bug. This way you do not care about needing to make assumptions about what the default project is, all this code will do is send straight through whatever is specified as BUG= in the individual CLs.
https://codereview.chromium.org/1849283002/diff/40001/go/util/util.go File go/util/util.go (right): https://codereview.chromium.org/1849283002/diff/40001/go/util/util.go#newcode636 go/util/util.go:636: return rv On 2016/04/04 12:45:09, rmistry wrote: > Why return a dictionary of projects to bugs? It would be simpler and still > correct to just parse out all strings delimited by ',' and return an array of > bugs which might contain contents like ["123", "chromium:123", "skia:123"]. You > can then blindly specify them in --bug. > This way you do not care about needing to make assumptions about what the > default project is, all this code will do is send straight through whatever is > specified as BUG= in the individual CLs. The autoroller only wants to specify chromium bugs. Others are not relevant to DEPS rolls. The caller could scan through and only take the "chromium:123" or "123" bugs, but it makes more sense to me to do it here. Plus, I'm doubtful that the default project will ever change.
On 2016/04/04 15:50:07, borenet wrote: > https://codereview.chromium.org/1849283002/diff/40001/go/util/util.go > File go/util/util.go (right): > > https://codereview.chromium.org/1849283002/diff/40001/go/util/util.go#newcode636 > go/util/util.go:636: return rv > On 2016/04/04 12:45:09, rmistry wrote: > > Why return a dictionary of projects to bugs? It would be simpler and still > > correct to just parse out all strings delimited by ',' and return an array of > > bugs which might contain contents like ["123", "chromium:123", "skia:123"]. > You > > can then blindly specify them in --bug. > > This way you do not care about needing to make assumptions about what the > > default project is, all this code will do is send straight through whatever is > > specified as BUG= in the individual CLs. > > The autoroller only wants to specify chromium bugs. Others are not relevant to > DEPS rolls. The caller could scan through and only take the "chromium:123" or > "123" bugs, but it makes more sense to me to do it here. Plus, I'm doubtful > that the default project will ever change. I do not follow. Why would autoroller only want to specify chromium bugs? If I fix something in catapult and mention a Skia bug I would want Bugdroid to update it when the DEPs roll lands.
On 2016/04/04 15:52:16, rmistry wrote: > On 2016/04/04 15:50:07, borenet wrote: > > https://codereview.chromium.org/1849283002/diff/40001/go/util/util.go > > File go/util/util.go (right): > > > > > https://codereview.chromium.org/1849283002/diff/40001/go/util/util.go#newcode636 > > go/util/util.go:636: return rv > > On 2016/04/04 12:45:09, rmistry wrote: > > > Why return a dictionary of projects to bugs? It would be simpler and still > > > correct to just parse out all strings delimited by ',' and return an array > of > > > bugs which might contain contents like ["123", "chromium:123", "skia:123"]. > > You > > > can then blindly specify them in --bug. > > > This way you do not care about needing to make assumptions about what the > > > default project is, all this code will do is send straight through whatever > is > > > specified as BUG= in the individual CLs. > > > > The autoroller only wants to specify chromium bugs. Others are not relevant > to > > DEPS rolls. The caller could scan through and only take the "chromium:123" or > > "123" bugs, but it makes more sense to me to do it here. Plus, I'm doubtful > > that the default project will ever change. > > I do not follow. Why would autoroller only want to specify chromium bugs? If I > fix something in catapult and mention a Skia bug I would want Bugdroid to update > it when the DEPs roll lands. Generally speaking, a Skia CL references a Skia bug. In those cases, bugdroid has already pinged the bug when the CL lands. The only reason to do it again when that CL rolls into chromium is if the bug is relevant to chromium, ie. a "chromium:123" bug. IMO pinging all bugs when the DEPS roll lands will just generate noise.
On 2016/04/04 16:31:32, borenet wrote: > On 2016/04/04 15:52:16, rmistry wrote: > > On 2016/04/04 15:50:07, borenet wrote: > > > https://codereview.chromium.org/1849283002/diff/40001/go/util/util.go > > > File go/util/util.go (right): > > > > > > > > > https://codereview.chromium.org/1849283002/diff/40001/go/util/util.go#newcode636 > > > go/util/util.go:636: return rv > > > On 2016/04/04 12:45:09, rmistry wrote: > > > > Why return a dictionary of projects to bugs? It would be simpler and still > > > > correct to just parse out all strings delimited by ',' and return an array > > of > > > > bugs which might contain contents like ["123", "chromium:123", > "skia:123"]. > > > You > > > > can then blindly specify them in --bug. > > > > This way you do not care about needing to make assumptions about what the > > > > default project is, all this code will do is send straight through > whatever > > is > > > > specified as BUG= in the individual CLs. > > > > > > The autoroller only wants to specify chromium bugs. Others are not relevant > > to > > > DEPS rolls. The caller could scan through and only take the "chromium:123" > or > > > "123" bugs, but it makes more sense to me to do it here. Plus, I'm doubtful > > > that the default project will ever change. > > > > I do not follow. Why would autoroller only want to specify chromium bugs? If I > > fix something in catapult and mention a Skia bug I would want Bugdroid to > update > > it when the DEPs roll lands. > > Generally speaking, a Skia CL references a Skia bug. In those cases, bugdroid > has already pinged the bug when the CL lands. The only reason to do it again > when that CL rolls into chromium is if the bug is relevant to chromium, ie. a > "chromium:123" bug. IMO pinging all bugs when the DEPS roll lands will just > generate noise. Ok I think understand the problem now. This is a workaround for the fact that catapult does not have a bugdroid instance. What about this scenario: When a Skia CL with a Chromium bug reference lands in Skia repo, the Chromium bug is updated. But when our DEPs roll lands it will be updated again?
On 2016/04/04 16:38:21, rmistry wrote: > On 2016/04/04 16:31:32, borenet wrote: > > On 2016/04/04 15:52:16, rmistry wrote: > > > On 2016/04/04 15:50:07, borenet wrote: > > > > https://codereview.chromium.org/1849283002/diff/40001/go/util/util.go > > > > File go/util/util.go (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1849283002/diff/40001/go/util/util.go#newcode636 > > > > go/util/util.go:636: return rv > > > > On 2016/04/04 12:45:09, rmistry wrote: > > > > > Why return a dictionary of projects to bugs? It would be simpler and > still > > > > > correct to just parse out all strings delimited by ',' and return an > array > > > of > > > > > bugs which might contain contents like ["123", "chromium:123", > > "skia:123"]. > > > > You > > > > > can then blindly specify them in --bug. > > > > > This way you do not care about needing to make assumptions about what > the > > > > > default project is, all this code will do is send straight through > > whatever > > > is > > > > > specified as BUG= in the individual CLs. > > > > > > > > The autoroller only wants to specify chromium bugs. Others are not > relevant > > > to > > > > DEPS rolls. The caller could scan through and only take the > "chromium:123" > > or > > > > "123" bugs, but it makes more sense to me to do it here. Plus, I'm > doubtful > > > > that the default project will ever change. > > > > > > I do not follow. Why would autoroller only want to specify chromium bugs? If > I > > > fix something in catapult and mention a Skia bug I would want Bugdroid to > > update > > > it when the DEPs roll lands. > > > > Generally speaking, a Skia CL references a Skia bug. In those cases, bugdroid > > has already pinged the bug when the CL lands. The only reason to do it again > > when that CL rolls into chromium is if the bug is relevant to chromium, ie. a > > "chromium:123" bug. IMO pinging all bugs when the DEPS roll lands will just > > generate noise. > > Ok I think understand the problem now. This is a workaround for the fact that > catapult does not have a bugdroid instance. > > What about this scenario: When a Skia CL with a Chromium bug reference lands in > Skia repo, the Chromium bug is updated. But when our DEPs roll lands it will be > updated again? AFAIK bugdroid does work for catapult. Your Skia example is correct. If I create a Skia CL, "Fix chromium bug #12345", it'll update chromium:12345 when it lands in the Skia repo. At the moment, that's all the information we get, essentially, "this bug will be fixed once this change gets rolled." But since the bug isn't really fixed until that CL rolls into chromium, it makes sense to update chromium:12345 again once the roll lands.
On 2016/04/04 16:43:10, borenet wrote: > On 2016/04/04 16:38:21, rmistry wrote: > > On 2016/04/04 16:31:32, borenet wrote: > > > On 2016/04/04 15:52:16, rmistry wrote: > > > > On 2016/04/04 15:50:07, borenet wrote: > > > > > https://codereview.chromium.org/1849283002/diff/40001/go/util/util.go > > > > > File go/util/util.go (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1849283002/diff/40001/go/util/util.go#newcode636 > > > > > go/util/util.go:636: return rv > > > > > On 2016/04/04 12:45:09, rmistry wrote: > > > > > > Why return a dictionary of projects to bugs? It would be simpler and > > still > > > > > > correct to just parse out all strings delimited by ',' and return an > > array > > > > of > > > > > > bugs which might contain contents like ["123", "chromium:123", > > > "skia:123"]. > > > > > You > > > > > > can then blindly specify them in --bug. > > > > > > This way you do not care about needing to make assumptions about what > > the > > > > > > default project is, all this code will do is send straight through > > > whatever > > > > is > > > > > > specified as BUG= in the individual CLs. > > > > > > > > > > The autoroller only wants to specify chromium bugs. Others are not > > relevant > > > > to > > > > > DEPS rolls. The caller could scan through and only take the > > "chromium:123" > > > or > > > > > "123" bugs, but it makes more sense to me to do it here. Plus, I'm > > doubtful > > > > > that the default project will ever change. > > > > > > > > I do not follow. Why would autoroller only want to specify chromium bugs? > If > > I > > > > fix something in catapult and mention a Skia bug I would want Bugdroid to > > > update > > > > it when the DEPs roll lands. > > > > > > Generally speaking, a Skia CL references a Skia bug. In those cases, > bugdroid > > > has already pinged the bug when the CL lands. The only reason to do it > again > > > when that CL rolls into chromium is if the bug is relevant to chromium, ie. > a > > > "chromium:123" bug. IMO pinging all bugs when the DEPS roll lands will just > > > generate noise. > > > > Ok I think understand the problem now. This is a workaround for the fact that > > catapult does not have a bugdroid instance. > > > > What about this scenario: When a Skia CL with a Chromium bug reference lands > in > > Skia repo, the Chromium bug is updated. But when our DEPs roll lands it will > be > > updated again? > > AFAIK bugdroid does work for catapult. Your Skia example is correct. If I > create a Skia CL, "Fix chromium bug #12345", it'll update chromium:12345 when it > lands in the Skia repo. At the moment, that's all the information we get, > essentially, "this bug will be fixed once this change gets rolled." But since > the bug isn't really fixed until that CL rolls into chromium, it makes sense to > update chromium:12345 again once the roll lands. Yes that makes sense to me now. LGTM.
The CQ bit was checked by borenet@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1849283002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1849283002/40001
Message was sent while issue was closed.
Description was changed from ========== AutoRolls include Chromium bug links BUG=598773 ========== to ========== AutoRolls include Chromium bug links BUG=598773 Committed: https://skia.googlesource.com/buildbot/+/2fb6667166f94531c6fd0df13570f5d1b650... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://skia.googlesource.com/buildbot/+/2fb6667166f94531c6fd0df13570f5d1b650... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
