Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2249983004:
Add apply_gerrit_ref to bot_update api (Closed)
|
Created:
4 years, 4 months ago by rmistry Modified:
4 years, 4 months ago Reviewers:
tandrii(chromium) CC:
chromium-reviews, dpranke+depot_tools_chromium.org, iannucci+depot_tools_chromium.org Base URL:
https://chromium.googlesource.com/chromium/tools/depot_tools.git@master Target Ref:
refs/heads/master Project:
depot_tools Visibility:
Public. |
DescriptionAdd apply_gerrit_ref to bot_update api.
BUG=skia:5627
Committed: https://chromium.googlesource.com/chromium/tools/depot_tools/+/2250d5b3b0a5686c83171dc2b9a1e66270c0c41d
Patch Set 1 : Initial upload #Patch Set 2 : Working e2e #Patch Set 3 : Use bot_update #
Total comments: 4
Patch Set 4 : Add new apply_gerrit_ref in bot_update api.py #
Total comments: 6
Patch Set 5 : Address comments #Patch Set 6 : Add tests #
Total comments: 2
Patch Set 7 : Address comment #
Total comments: 2
Patch Set 8 : disable=relative-import #
Messages
Total messages: 38 (18 generated)
Description was changed from ========== depot_tools checkpoint BUG= ========== to ========== depot_tools tryserver/api.py checkpoint BUG= ==========
Description was changed from ========== depot_tools tryserver/api.py checkpoint BUG= ========== to ========== Support Gerrit patches in recipe module tryserver:maybe_apply_issue. BUG=chromium:631528 ==========
Description was changed from ========== Support Gerrit patches in recipe module tryserver:maybe_apply_issue. BUG=chromium:631528 ========== to ========== Support Gerrit patches in recipe module tryserver:maybe_apply_issue. Skia does plan to eventually update our code to use bot_update (https://bugs.chromium.org/p/skia/issues/detail?id=5588) but we would like to be able to use Gerrit via maybe_apply_issue till then. BUG=chromium:631528 ==========
rmistry@google.com changed reviewers: + tandrii@chromium.org
No tests yet. Wanted to first see what you thought about this. https://codereview.chromium.org/2249983004/diff/40001/recipe_modules/tryserve... File recipe_modules/tryserver/api.py (right): https://codereview.chromium.org/2249983004/diff/40001/recipe_modules/tryserve... recipe_modules/tryserver/api.py:19: import bot_update Did not want to duplicate all the work already done in https://cs.chromium.org/chromium/tools/depot_tools/recipe_modules/bot_update/... Which is why I am instead calling it from here.
i don't want to discourage you, but I think this is exactly what recipes was supposed to discourage. I could be wrong, so feel free to ask someone else. also, very IMO, https://bugs.chromium.org/p/skia/issues/detail?id=5588 seems much easier to do assuming your recipe isn't doing fancy stuff with got_revision[_*] properties, though YEAH, it's risky (thus maybe put it behind a flag?) https://codereview.chromium.org/2249983004/diff/40001/recipe_modules/tryserve... File recipe_modules/tryserver/api.py (right): https://codereview.chromium.org/2249983004/diff/40001/recipe_modules/tryserve... recipe_modules/tryserver/api.py:19: import bot_update On 2016/08/18 18:39:51, rmistry wrote: > Did not want to duplicate all the work already done in > https://cs.chromium.org/chromium/tools/depot_tools/recipe_modules/bot_update/... > Which is why I am instead calling it from here. that's bad. And it's against the spirit of recipes, and if it works, it's a bug.
On 2016/08/18 18:54:13, tandrii(chromium) wrote: > i don't want to discourage you, but I think this is exactly what recipes was > supposed to discourage. I could be wrong, so feel free to ask someone else. No need, I believe you and I strongly suspected that when I was hacking up the hack :) > > also, very IMO, https://bugs.chromium.org/p/skia/issues/detail?id=5588 seems > much easier to do assuming your recipe isn't doing fancy stuff with > got_revision[_*] properties, though YEAH, it's risky (thus maybe put it behind a > flag?) Yea, I actually did try this out in https://codereview.chromium.org/2242213007/ But there are various nuances and issues that we have previously run into with checkouts that I would much rather borenet@ tackle it in https://bugs.chromium.org/p/skia/issues/detail?id=5588. He also has the context of all the issues we have run into in the past and would be the best person to tackle in. > > https://codereview.chromium.org/2249983004/diff/40001/recipe_modules/tryserve... > File recipe_modules/tryserver/api.py (right): > > https://codereview.chromium.org/2249983004/diff/40001/recipe_modules/tryserve... > recipe_modules/tryserver/api.py:19: import bot_update > On 2016/08/18 18:39:51, rmistry wrote: > > Did not want to duplicate all the work already done in > > > https://cs.chromium.org/chromium/tools/depot_tools/recipe_modules/bot_update/... > > Which is why I am instead calling it from here. > > that's bad. And it's against the spirit of recipes, and if it works, it's a bug.
https://codereview.chromium.org/2249983004/diff/40001/recipe_modules/tryserve... File recipe_modules/tryserver/api.py (right): https://codereview.chromium.org/2249983004/diff/40001/recipe_modules/tryserve... recipe_modules/tryserver/api.py:19: import bot_update On 2016/08/18 18:54:13, tandrii(chromium) wrote: > On 2016/08/18 18:39:51, rmistry wrote: > > Did not want to duplicate all the work already done in > > > https://cs.chromium.org/chromium/tools/depot_tools/recipe_modules/bot_update/... > > Which is why I am instead calling it from here. > > that's bad. And it's against the spirit of recipes, and if it works, it's a bug. It does work (I tried it out locally). Could I instead extract out apply_gerrit into it's own recipe_module and use it from tryserver and bot_update ? Actually not sure if that would work since bot_update here is a resource and not a recipe_module. Any other suggestions?
https://codereview.chromium.org/2249983004/diff/40001/recipe_modules/tryserve... File recipe_modules/tryserver/api.py (right): https://codereview.chromium.org/2249983004/diff/40001/recipe_modules/tryserve... recipe_modules/tryserver/api.py:19: import bot_update On 2016/08/18 20:27:50, rmistry wrote: > On 2016/08/18 18:54:13, tandrii(chromium) wrote: > > On 2016/08/18 18:39:51, rmistry wrote: > > > Did not want to duplicate all the work already done in > > > > > > https://cs.chromium.org/chromium/tools/depot_tools/recipe_modules/bot_update/... > > > Which is why I am instead calling it from here. > > > > that's bad. And it's against the spirit of recipes, and if it works, it's a > bug. > > It does work (I tried it out locally). > > Could I instead extract out apply_gerrit into it's own recipe_module and use it > from tryserver and bot_update ? > Actually not sure if that would work since bot_update here is a resource and not > a recipe_module. Any other suggestions? That OR wdyt about adding new API in bot_update + new resource file in bot_update, say apply_gerrit.py which: # That's allowed, so long as you have __init__.py, import bot_update if __name__ == '__main__': bot_update.whatever_you_need
On 2016/08/18 20:30:09, tandrii(chromium) wrote: > https://codereview.chromium.org/2249983004/diff/40001/recipe_modules/tryserve... > File recipe_modules/tryserver/api.py (right): > > https://codereview.chromium.org/2249983004/diff/40001/recipe_modules/tryserve... > recipe_modules/tryserver/api.py:19: import bot_update > On 2016/08/18 20:27:50, rmistry wrote: > > On 2016/08/18 18:54:13, tandrii(chromium) wrote: > > > On 2016/08/18 18:39:51, rmistry wrote: > > > > Did not want to duplicate all the work already done in > > > > > > > > > > https://cs.chromium.org/chromium/tools/depot_tools/recipe_modules/bot_update/... > > > > Which is why I am instead calling it from here. > > > > > > that's bad. And it's against the spirit of recipes, and if it works, it's a > > bug. > > > > It does work (I tried it out locally). > > > > Could I instead extract out apply_gerrit into it's own recipe_module and use > it > > from tryserver and bot_update ? > > Actually not sure if that would work since bot_update here is a resource and > not > > a recipe_module. Any other suggestions? > > That OR wdyt about adding new API in bot_update + new resource file in > bot_update, say apply_gerrit.py which: > > # That's allowed, so long as you have __init__.py, > import bot_update > if __name__ == '__main__': > bot_update.whatever_you_need That sounds promising but making sure I understand: * Create a new resource file "apply_gerrit.py" * Add new method in bot_update recipe_module. Eg "def apply_gerrit". Call the new resource file from this method. * Everything else stays the same (i.e. the current bot_update resource is not modified).
On 2016/08/18 20:42:11, rmistry wrote: > On 2016/08/18 20:30:09, tandrii(chromium) wrote: > > > https://codereview.chromium.org/2249983004/diff/40001/recipe_modules/tryserve... > > File recipe_modules/tryserver/api.py (right): > > > > > https://codereview.chromium.org/2249983004/diff/40001/recipe_modules/tryserve... > > recipe_modules/tryserver/api.py:19: import bot_update > > On 2016/08/18 20:27:50, rmistry wrote: > > > On 2016/08/18 18:54:13, tandrii(chromium) wrote: > > > > On 2016/08/18 18:39:51, rmistry wrote: > > > > > Did not want to duplicate all the work already done in > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/tools/depot_tools/recipe_modules/bot_update/... > > > > > Which is why I am instead calling it from here. > > > > > > > > that's bad. And it's against the spirit of recipes, and if it works, it's > a > > > bug. > > > > > > It does work (I tried it out locally). > > > > > > Could I instead extract out apply_gerrit into it's own recipe_module and use > > it > > > from tryserver and bot_update ? > > > Actually not sure if that would work since bot_update here is a resource and > > not > > > a recipe_module. Any other suggestions? > > > > That OR wdyt about adding new API in bot_update + new resource file in > > bot_update, say apply_gerrit.py which: > > > > # That's allowed, so long as you have __init__.py, > > import bot_update > > if __name__ == '__main__': > > bot_update.whatever_you_need > > That sounds promising but making sure I understand: > > * Create a new resource file "apply_gerrit.py" > * Add new method in bot_update recipe_module. Eg "def apply_gerrit". Call the > new resource file from this method. > * Everything else stays the same (i.e. the current bot_update resource is not > modified). Yep, that's what I meant. I'd call it "do_not_use_apply_gerrit_ref" with #TODO remove by Q4 2016 :)
Patchset #4 (id:60001) has been deleted
On 2016/08/18 20:46:13, tandrii(chromium) wrote: > On 2016/08/18 20:42:11, rmistry wrote: > > On 2016/08/18 20:30:09, tandrii(chromium) wrote: > > > > > > https://codereview.chromium.org/2249983004/diff/40001/recipe_modules/tryserve... > > > File recipe_modules/tryserver/api.py (right): > > > > > > > > > https://codereview.chromium.org/2249983004/diff/40001/recipe_modules/tryserve... > > > recipe_modules/tryserver/api.py:19: import bot_update > > > On 2016/08/18 20:27:50, rmistry wrote: > > > > On 2016/08/18 18:54:13, tandrii(chromium) wrote: > > > > > On 2016/08/18 18:39:51, rmistry wrote: > > > > > > Did not want to duplicate all the work already done in > > > > > > > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/tools/depot_tools/recipe_modules/bot_update/... > > > > > > Which is why I am instead calling it from here. > > > > > > > > > > that's bad. And it's against the spirit of recipes, and if it works, > it's > > a > > > > bug. > > > > > > > > It does work (I tried it out locally). > > > > > > > > Could I instead extract out apply_gerrit into it's own recipe_module and > use > > > it > > > > from tryserver and bot_update ? > > > > Actually not sure if that would work since bot_update here is a resource > and > > > not > > > > a recipe_module. Any other suggestions? > > > > > > That OR wdyt about adding new API in bot_update + new resource file in > > > bot_update, say apply_gerrit.py which: > > > > > > # That's allowed, so long as you have __init__.py, > > > import bot_update > > > if __name__ == '__main__': > > > bot_update.whatever_you_need > > > > That sounds promising but making sure I understand: > > > > * Create a new resource file "apply_gerrit.py" > > * Add new method in bot_update recipe_module. Eg "def apply_gerrit". Call the > > new resource file from this method. > > * Everything else stays the same (i.e. the current bot_update resource is not > > modified). > > Yep, that's what I meant. I'd call it "do_not_use_apply_gerrit_ref" with #TODO > remove by Q4 2016 :) So I implemented this @patchset4 but it does not work because of cyclic dependencies: * tryserver depends on bot_update which depends on tryserver * tryserver depends on bot_update which depends on gclient which depends on triserver
yep, that's what I meant. https://codereview.chromium.org/2249983004/diff/80001/recipe_modules/bot_upda... File recipe_modules/bot_update/api.py (right): https://codereview.chromium.org/2249983004/diff/80001/recipe_modules/bot_upda... recipe_modules/bot_update/api.py:56: # DO NOT USE. please add bug URL. https://codereview.chromium.org/2249983004/diff/80001/recipe_modules/bot_upda... File recipe_modules/bot_update/resources/apply_gerrit.py (right): https://codereview.chromium.org/2249983004/diff/80001/recipe_modules/bot_upda... recipe_modules/bot_update/resources/apply_gerrit.py:6: import optparse nitty: argparse is nicer, but don't change now :) https://codereview.chromium.org/2249983004/diff/80001/recipe_modules/tryserve... File recipe_modules/tryserver/api.py (right): https://codereview.chromium.org/2249983004/diff/80001/recipe_modules/tryserve... recipe_modules/tryserver/api.py:147: elif storage == PATCH_STORAGE_GERRIT: Hm, can you move this condition instead to skia recipe just to get going?
Patchset #5 (id:100001) has been deleted
Patchset #5 (id:120001) has been deleted
https://codereview.chromium.org/2249983004/diff/80001/recipe_modules/bot_upda... File recipe_modules/bot_update/api.py (right): https://codereview.chromium.org/2249983004/diff/80001/recipe_modules/bot_upda... recipe_modules/bot_update/api.py:56: # DO NOT USE. On 2016/08/19 14:00:04, tandrii(chromium) wrote: > please add bug URL. Done. https://codereview.chromium.org/2249983004/diff/80001/recipe_modules/bot_upda... File recipe_modules/bot_update/resources/apply_gerrit.py (right): https://codereview.chromium.org/2249983004/diff/80001/recipe_modules/bot_upda... recipe_modules/bot_update/resources/apply_gerrit.py:6: import optparse On 2016/08/19 14:00:04, tandrii(chromium) wrote: > nitty: argparse is nicer, but don't change now :) Acknowledged. https://codereview.chromium.org/2249983004/diff/80001/recipe_modules/tryserve... File recipe_modules/tryserver/api.py (right): https://codereview.chromium.org/2249983004/diff/80001/recipe_modules/tryserve... recipe_modules/tryserver/api.py:147: elif storage == PATCH_STORAGE_GERRIT: On 2016/08/19 14:00:04, tandrii(chromium) wrote: > Hm, can you move this condition instead to skia recipe just to get going? That makes sense. Will do.
Cleaned up and added tests @Patchset6. PTAL.
Description was changed from ========== Support Gerrit patches in recipe module tryserver:maybe_apply_issue. Skia does plan to eventually update our code to use bot_update (https://bugs.chromium.org/p/skia/issues/detail?id=5588) but we would like to be able to use Gerrit via maybe_apply_issue till then. BUG=chromium:631528 ========== to ========== Add apply_gerrit_ref to bot_update api. BUG=chromium:631528 ==========
Description was changed from ========== Add apply_gerrit_ref to bot_update api. BUG=chromium:631528 ========== to ========== Add apply_gerrit_ref to bot_update api. BUG=skia:5627 ==========
lgtm https://codereview.chromium.org/2249983004/diff/160001/recipe_modules/bot_upd... File recipe_modules/bot_update/api.py (right): https://codereview.chromium.org/2249983004/diff/160001/recipe_modules/bot_upd... recipe_modules/bot_update/api.py:64: kwargs['env'].setdefault('PATH', '%(PATH)s') Just because I can :) kwargs.setdefault('env', {}).setdefault('PATH', '%(PATH)s')
https://codereview.chromium.org/2249983004/diff/160001/recipe_modules/bot_upd... File recipe_modules/bot_update/api.py (right): https://codereview.chromium.org/2249983004/diff/160001/recipe_modules/bot_upd... recipe_modules/bot_update/api.py:64: kwargs['env'].setdefault('PATH', '%(PATH)s') On 2016/08/19 17:50:42, tandrii(chromium) wrote: > Just because I can :) > > kwargs.setdefault('env', {}).setdefault('PATH', '%(PATH)s') Done.
The CQ bit was checked by rmistry@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from tandrii@chromium.org Link to the patchset: https://codereview.chromium.org/2249983004/#ps180001 (title: "Address comment")
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: Depot Tools Presubmit on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/30bcd61670673c10)
https://codereview.chromium.org/2249983004/diff/180001/recipe_modules/bot_upd... File recipe_modules/bot_update/resources/apply_gerrit.py (right): https://codereview.chromium.org/2249983004/diff/180001/recipe_modules/bot_upd... recipe_modules/bot_update/resources/apply_gerrit.py:9: import bot_update Looks like this is giving a pylint error: ** Presubmit ERRORS ** Pylint (176 files on 4 cores) (19.18s) failed ************* Module bot_update.resources.apply_gerrit W: 9, 0: Relative import 'bot_update', should be 'bot_update.resources.bot_update' (relative-import) I tried different things here but they did not work: from . import bot_update from bot_update.resources import bot_update import bot_update.resources.bot_update Any ideas?
https://codereview.chromium.org/2249983004/diff/180001/recipe_modules/bot_upd... File recipe_modules/bot_update/resources/apply_gerrit.py (right): https://codereview.chromium.org/2249983004/diff/180001/recipe_modules/bot_upd... recipe_modules/bot_update/resources/apply_gerrit.py:9: import bot_update On 2016/08/19 18:32:34, rmistry wrote: > Looks like this is giving a pylint error: > > ** Presubmit ERRORS ** > Pylint (176 files on 4 cores) (19.18s) failed > ************* Module bot_update.resources.apply_gerrit > W: 9, 0: Relative import 'bot_update', should be > 'bot_update.resources.bot_update' (relative-import) > > > I tried different things here but they did not work: > > from . import bot_update > from bot_update.resources import bot_update > import bot_update.resources.bot_update > > Any ideas? Added disable=relative-import since saw it used in some resources here: https://cs.chromium.org/chromium/build/scripts/slave/recipe_modules/auto_bise... Could not find an example in other resources with relative imports. Let me know what you think.
The CQ bit was checked by rmistry@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.
LGTM
The CQ bit was checked by rmistry@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add apply_gerrit_ref to bot_update api. BUG=skia:5627 ========== to ========== Add apply_gerrit_ref to bot_update api. BUG=skia:5627 Committed: https://chromium.googlesource.com/chromium/tools/depot_tools/+/2250d5b3b0a568... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:200001) as https://chromium.googlesource.com/chromium/tools/depot_tools/+/2250d5b3b0a568... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
