Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11348122:
Add presubmit check to verify issue is not closed. (Closed)
|
Created:
8 years, 1 month ago by Isaac (away) Modified:
8 years ago CC:
chromium-reviews, Dirk Pranke, cmp+cc_chromium.org, iannucci Visibility:
Public. |
DescriptionAdd presubmit check to verify issue is not closed.
This can come up when CQ closed an issue but the user's
local branch is still tied to the issue.
BUG=161702
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=171153
Patch Set 1 : #
Total comments: 6
Patch Set 2 : addr M-A comments, refactored rietveld call #
Total comments: 5
Patch Set 3 : changed CheckOwners to be backwards compitable. More changes to rietveld.py #
Total comments: 4
Patch Set 4 : #
Total comments: 4
Patch Set 5 : #Messages
Total messages: 20 (0 generated)
I thought git-cl was already looking for it but I'm not 100% sure, maybe just gcl? https://chromiumcodereview.appspot.com/11348122/diff/2002/presubmit_canned_ch... File presubmit_canned_checks.py (right): https://chromiumcodereview.appspot.com/11348122/diff/2002/presubmit_canned_ch... presubmit_canned_checks.py:803: issue_props = input_api.rietveld.get_issue_properties( input_api.rietveld could be None in the case it's gerrit CL IIRC, so on 802, check if issue and input_api.rietveld: https://chromiumcodereview.appspot.com/11348122/diff/2002/presubmit_canned_ch... presubmit_canned_checks.py:804: issue=int(issue), messages=False) Is the int() call necessary? If so, it's a bug in the Change object IMHO. https://chromiumcodereview.appspot.com/11348122/diff/2002/presubmit_canned_ch... presubmit_canned_checks.py:812: 2 lines
It's not blocking it -- I checked. But git cl dcommit does reset the issue number on the current branch, so this mostly occurs with CQ. One thing I could change is to combine the rietveld query for owners and for this, if you think is necessary.
lgtm w/ maruel's comments addressed.
On 2012/11/19 19:57:06, Dirk Pranke wrote: > lgtm w/ maruel's comments addressed. Waiting for next patchset.
issues addressed + a refactor to avoid the extra rietveld call. Ready for review. https://chromiumcodereview.appspot.com/11348122/diff/2002/presubmit_canned_ch... File presubmit_canned_checks.py (right): https://chromiumcodereview.appspot.com/11348122/diff/2002/presubmit_canned_ch... presubmit_canned_checks.py:803: issue_props = input_api.rietveld.get_issue_properties( On 2012/11/19 13:04:01, Marc-Antoine Ruel wrote: > input_api.rietveld could be None in the case it's gerrit CL IIRC, so on 802, > check if issue and input_api.rietveld: Done, added to _GetRietveldIssueProps in latest patchset. https://chromiumcodereview.appspot.com/11348122/diff/2002/presubmit_canned_ch... presubmit_canned_checks.py:804: issue=int(issue), messages=False) On 2012/11/19 13:04:01, Marc-Antoine Ruel wrote: > Is the int() call necessary? If so, it's a bug in the Change object IMHO. It was required, fixed in latest patchset. https://chromiumcodereview.appspot.com/11348122/diff/2002/presubmit_canned_ch... presubmit_canned_checks.py:812: On 2012/11/19 13:04:01, Marc-Antoine Ruel wrote: > 2 lines Done.
https://chromiumcodereview.appspot.com/11348122/diff/1004/presubmit_canned_ch... File presubmit_canned_checks.py (right): https://chromiumcodereview.appspot.com/11348122/diff/1004/presubmit_canned_ch... presubmit_canned_checks.py:745: def CheckOwners(input_api, output_api, issue_props, source_file_filter=None): https://code.google.com/p/chromium/source/search?q=CheckOwners you'll break depot_tools and v8 presubmit checks. https://chromiumcodereview.appspot.com/11348122/diff/1004/presubmit_canned_ch... presubmit_canned_checks.py:798: return input_api.rietveld.get_issue_properties( BTW, this could be cached in the interface instead. Roughyl, create a Rietveld subclass that caches get_issue_properties return values. https://chromiumcodereview.appspot.com/11348122/diff/1004/rietveld.py File rietveld.py (right): https://chromiumcodereview.appspot.com/11348122/diff/1004/rietveld.py#newcode89 rietveld.py:89: url = '/api/%s' % issue Why? It was %d on purpose.
Made changes, ptal. https://chromiumcodereview.appspot.com/11348122/diff/1004/presubmit_canned_ch... File presubmit_canned_checks.py (right): https://chromiumcodereview.appspot.com/11348122/diff/1004/presubmit_canned_ch... presubmit_canned_checks.py:745: def CheckOwners(input_api, output_api, issue_props, source_file_filter=None): On 2012/11/21 20:48:38, Marc-Antoine Ruel wrote: > https://code.google.com/p/chromium/source/search?q=CheckOwners > > you'll break depot_tools and v8 presubmit checks. Changed to be an optional argument and moved to the last position to maintain positional order. https://chromiumcodereview.appspot.com/11348122/diff/1004/presubmit_canned_ch... presubmit_canned_checks.py:798: return input_api.rietveld.get_issue_properties( On 2012/11/21 20:48:38, Marc-Antoine Ruel wrote: > BTW, this could be cached in the interface instead. Roughyl, create a Rietveld > subclass that caches get_issue_properties return values. Good idea, but I'd like to punt on that. Also it would require special knowledge for this call because messages=True returns a super-set of the messages=False info.
On 2012/11/22 06:53:15, Isaac wrote: > Good idea, but I'd like to punt on that. Also it would require special > knowledge for this call because messages=True returns a super-set of the > messages=False info. I prefer to keep the caching at the root level, so I sent https://codereview.chromium.org/11280143
OK; will wait for that to land.
Mmmkay. I tried again to upload to a closed issue and this time got an error. Looking codereview/views.py::upload() in rietveld and third_party/upload.py in build repo; rietveld will block an upload to a closed issue. I can't figure out how my upload test in the past could have worked. This is still a valuable as a commit-only presubmit check, though.
https://chromiumcodereview.appspot.com/11348122/diff/9005/presubmit_canned_ch... File presubmit_canned_checks.py (right): https://chromiumcodereview.appspot.com/11348122/diff/9005/presubmit_canned_ch... presubmit_canned_checks.py:746: issue_props=None): Can you rebase on ToT and remove this argument now? https://chromiumcodereview.appspot.com/11348122/diff/9005/rietveld.py File rietveld.py (right): https://chromiumcodereview.appspot.com/11348122/diff/9005/rietveld.py#newcode80 rietveld.py:80: logging.info('closing issue %s' % issue) Please don't change these and revert everything in this file.
OK, vastly simplified this CL, ptal :-) https://chromiumcodereview.appspot.com/11348122/diff/9005/presubmit_canned_ch... File presubmit_canned_checks.py (right): https://chromiumcodereview.appspot.com/11348122/diff/9005/presubmit_canned_ch... presubmit_canned_checks.py:746: issue_props=None): On 2012/11/28 02:43:49, Marc-Antoine Ruel wrote: > Can you rebase on ToT and remove this argument now? done. https://chromiumcodereview.appspot.com/11348122/diff/9005/rietveld.py File rietveld.py (right): https://chromiumcodereview.appspot.com/11348122/diff/9005/rietveld.py#newcode80 rietveld.py:80: logging.info('closing issue %s' % issue) On 2012/11/28 02:43:49, Marc-Antoine Ruel wrote: > Please don't change these and revert everything in this file. OK... I thought you asked for that earlier when you said rietveld.py should be able to take an issue number as a string?
ping?
lgtm with nit https://chromiumcodereview.appspot.com/11348122/diff/4006/presubmit_canned_ch... File presubmit_canned_checks.py (right): https://chromiumcodereview.appspot.com/11348122/diff/4006/presubmit_canned_ch... presubmit_canned_checks.py:803: """Verify issue is not closed. Verifies https://chromiumcodereview.appspot.com/11348122/diff/4006/presubmit_canned_ch... presubmit_canned_checks.py:964: input_api=input_api, Personally I wouldn't change this line anymore, it's not neede.
nits addressed, committing https://chromiumcodereview.appspot.com/11348122/diff/4006/presubmit_canned_ch... File presubmit_canned_checks.py (right): https://chromiumcodereview.appspot.com/11348122/diff/4006/presubmit_canned_ch... presubmit_canned_checks.py:803: """Verify issue is not closed. On 2012/12/04 11:52:52, Marc-Antoine Ruel wrote: > Verifies Done. https://chromiumcodereview.appspot.com/11348122/diff/4006/presubmit_canned_ch... presubmit_canned_checks.py:964: input_api=input_api, On 2012/12/04 11:52:52, Marc-Antoine Ruel wrote: > Personally I wouldn't change this line anymore, it's not neede. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ilevy@chromium.org/11348122/14001
Message was sent while issue was closed.
Change committed as 171153
Message was sent while issue was closed.
Would have been nice to get a heads up that Robbie was doing something very similar! https://codereview.appspot.com/6851091/ Oh well, they are complementary... Explains msg11 though!
Message was sent while issue was closed.
On 2012/12/05 09:54:09, Isaac wrote: > Would have been nice to get a heads up that Robbie was doing something very > similar! https://codereview.appspot.com/6851091/ > > Oh well, they are complementary... Explains msg11 though! Oh sorry, totally forgot to make the link. |
