Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2089493002:
SOM: Fix key bug (Closed)
DescriptionSOM: Fix key bug
BUG=621710
Committed: https://chromium.googlesource.com/infra/infra/+/5af98b5538997aee5f13807972a03db8e277fd10
Patch Set 1 #
Total comments: 1
Patch Set 2 : Changed _keyForAlert() to just alert.key #Messages
Total messages: 18 (7 generated)
zhangtiff@google.com changed reviewers: + martiniss@chromium.org
PTAL
martiniss@chromium.org changed reviewers: + seanmccullough@chromium.org
Sean, WDYT about generating a key if an alert doesn't have a key. Is it guaranteed to always have one now?
Is there a bug for this, just so I understand the problem it fixes?
Description was changed from ========== I think this should fix the key bug? BUG= ========== to ========== I think this should fix the key bug? BUG=621710 ==========
Added a bug.
seanmccullough@google.com changed reviewers: + seanmccullough@google.com
https://codereview.chromium.org/2089493002/diff/1/go/src/infra/appengine/sher... File go/src/infra/appengine/sheriff-o-matic/elements/som-app.html (right): https://codereview.chromium.org/2089493002/diff/1/go/src/infra/appengine/sher... go/src/infra/appengine/sheriff-o-matic/elements/som-app.html:493: } else if (alert.extension && alert.extension.reasons && ok I see. Is it okay to just use alert.key instead of calling _keyForAlert() at this point?
On 2016/06/20 at 23:28:02, seanmccullough wrote: > https://codereview.chromium.org/2089493002/diff/1/go/src/infra/appengine/sher... > File go/src/infra/appengine/sheriff-o-matic/elements/som-app.html (right): > > https://codereview.chromium.org/2089493002/diff/1/go/src/infra/appengine/sher... > go/src/infra/appengine/sheriff-o-matic/elements/som-app.html:493: } else if (alert.extension && alert.extension.reasons && > ok I see. Is it okay to just use alert.key instead of calling _keyForAlert() at this point? If alert.key will always be set, then that would work. I can make that change if desired.
On 2016/06/20 23:35:28, Tiff wrote: > On 2016/06/20 at 23:28:02, seanmccullough wrote: > > > https://codereview.chromium.org/2089493002/diff/1/go/src/infra/appengine/sher... > > File go/src/infra/appengine/sheriff-o-matic/elements/som-app.html (right): > > > > > https://codereview.chromium.org/2089493002/diff/1/go/src/infra/appengine/sher... > > go/src/infra/appengine/sheriff-o-matic/elements/som-app.html:493: } else if > (alert.extension && alert.extension.reasons && > > ok I see. Is it okay to just use alert.key instead of calling _keyForAlert() > at this point? > > If alert.key will always be set, then that would work. I can make that change if > desired. We <3 fixing things and deleting code at the same time! :) Make sure tests pass of course.
On 2016/06/20 at 23:50:41, seanmccullough wrote: > On 2016/06/20 23:35:28, Tiff wrote: > > On 2016/06/20 at 23:28:02, seanmccullough wrote: > > > > > https://codereview.chromium.org/2089493002/diff/1/go/src/infra/appengine/sher... > > > File go/src/infra/appengine/sheriff-o-matic/elements/som-app.html (right): > > > > > > > > https://codereview.chromium.org/2089493002/diff/1/go/src/infra/appengine/sher... > > > go/src/infra/appengine/sheriff-o-matic/elements/som-app.html:493: } else if > > (alert.extension && alert.extension.reasons && > > > ok I see. Is it okay to just use alert.key instead of calling _keyForAlert() > > at this point? > > > > If alert.key will always be set, then that would work. I can make that change if > > desired. > > We <3 fixing things and deleting code at the same time! :) Make sure tests pass of course. Deleted _keyForAlert() and changed all calls to just use alert.key instead. I ran the tests and got two failures, but the failures seem to happen on the master branch without my changes, so I think this is passing.
lgtm
The CQ bit was checked by zhangtiff@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2089493002/20001
Description was changed from ========== I think this should fix the key bug? BUG=621710 ========== to ========== SOM: Fix key bug BUG=621710 ==========
Message was sent while issue was closed.
Description was changed from ========== SOM: Fix key bug BUG=621710 ========== to ========== SOM: Fix key bug BUG=621710 Committed: https://chromium.googlesource.com/infra/infra/+/5af98b5538997aee5f13807972a03... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/infra/infra/+/5af98b5538997aee5f13807972a03... |
