Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2993623002:
Milo: Limit text field to 256 characters (Closed)
|
Created:
3 years, 4 months ago by Ryan Tseng Modified:
3 years, 4 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: Limit text field to 256 characters
To fix the string of 500 errors in the pubsub handler.
BUG=
Review-Url: https://codereview.chromium.org/2993623002
Committed: https://github.com/luci/luci-go/commit/ff21376e5dded7b6c27746006ea432364243f51d
Patch Set 1 #Patch Set 2 : Typo #
Total comments: 4
Patch Set 3 : Review #Patch Set 4 : Go Build #Patch Set 5 : go fmt #Messages
Total messages: 32 (25 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...
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.
hinoka@google.com changed reviewers: + dnj@chromium.org, iannucci@chromium.org
ptal this may turn into an alert soon
lgtm w/ comment https://codereview.chromium.org/2993623002/diff/20001/milo/api/resp/build.go File milo/api/resp/build.go (right): https://codereview.chromium.org/2993623002/diff/20001/milo/api/resp/build.go#... milo/api/resp/build.go:290: // we don't really need anything much longer than 200 char or so. s/200/256? https://codereview.chromium.org/2993623002/diff/20001/milo/api/resp/build.go#... milo/api/resp/build.go:293: text = text[0:256] nit: can just do text[:256]. However, there is a problem here, namely that "text" can be unicode, in which case this may slice a UTF8 rune in half. Since the GAE limit is bytes, you want up to 256 bytes worth of runes. This walks over each rune in the string and records the last rune offset whose position is <256. sliceIdx := 0 for idx, r := range text { if idx > 256 { break } sliceIdx = idx } text = text[:sliceIdx]
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...
https://codereview.chromium.org/2993623002/diff/20001/milo/api/resp/build.go File milo/api/resp/build.go (right): https://codereview.chromium.org/2993623002/diff/20001/milo/api/resp/build.go#... milo/api/resp/build.go:290: // we don't really need anything much longer than 200 char or so. On 2017/08/03 16:28:20, dnj wrote: > s/200/256? I was trying to highlight the inexact nature of what the max len could be, but I guess that deters. https://codereview.chromium.org/2993623002/diff/20001/milo/api/resp/build.go#... milo/api/resp/build.go:293: text = text[0:256] On 2017/08/03 16:28:20, dnj wrote: > nit: can just do text[:256]. > > However, there is a problem here, namely that "text" can be unicode, in which > case this may slice a UTF8 rune in half. > > Since the GAE limit is bytes, you want up to 256 bytes worth of runes. This > walks over each rune in the string and records the last rune offset whose > position is <256. > > sliceIdx := 0 > for idx, r := range text { > if idx > 256 { > break > } > sliceIdx = idx > } > text = text[:sliceIdx] Weird, Go used to complain if I did [:number] done.
The CQ bit was unchecked by hinoka@google.com
The CQ bit was checked by hinoka@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from dnj@chromium.org Link to the patchset: https://codereview.chromium.org/2993623002/#ps40001 (title: "Review")
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
Try jobs failed on following builders: Luci-go Linux Trusty 64 Tester on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/37c1c4853f445210)
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: Try jobs failed on following builders: Luci-go Presubmit on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/37c1c9b4b4c0c910)
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.
The CQ bit was checked by hinoka@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from dnj@chromium.org Link to the patchset: https://codereview.chromium.org/2993623002/#ps80001 (title: "go fmt")
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": 80001, "attempt_start_ts": 1501779132744550,
"parent_rev": "9df595f68be87fb61fa58b72c9f7991d9d2eb2af", "commit_rev":
"ff21376e5dded7b6c27746006ea432364243f51d"}
Message was sent while issue was closed.
Description was changed from ========== Milo: Limit text field to 256 characters To fix the string of 500 errors in the pubsub handler. BUG= ========== to ========== Milo: Limit text field to 256 characters To fix the string of 500 errors in the pubsub handler. BUG= Review-Url: https://codereview.chromium.org/2993623002 Committed: https://github.com/luci/luci-go/commit/ff21376e5dded7b6c27746006ea432364243f51d ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://github.com/luci/luci-go/commit/ff21376e5dded7b6c27746006ea432364243f51d |
