Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1741983002:
bot_update: Rewrite to use properties, and add override options for patches. (Closed)
|
Created:
4 years, 10 months ago by martiniss Modified:
4 years, 9 months ago 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. |
Descriptionbot_update: Rewrite to use properties, and add override options for patches.
Depends on https://codereview.chromium.org/1755593003
BUG=591172
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=299332
Patch Set 1 #Patch Set 2 : Rewrote module to use properties. #
Total comments: 5
Patch Set 3 : Changes to the API, and comments. #
Total comments: 2
Patch Set 4 : Rebase and patches. #Patch Set 5 : Small cleanup. #Patch Set 6 : Add szager change, fix small bug. #
Total comments: 9
Patch Set 7 : Split out CLs. #Patch Set 8 : Split up CLs moar #Patch Set 9 : Rebase. #Messages
Total messages: 30 (10 generated)
martiniss@chromium.org changed reviewers: + iannucci@chromium.org
PTAL
On 2016/02/26 at 23:51:01, martiniss wrote: > PTAL No expectation changes? How?
On 2016/02/27 at 00:42:20, iannucci wrote: > On 2016/02/26 at 23:51:01, martiniss wrote: > > PTAL > > No expectation changes? How? Umm, not sure? It should be transparent.... Also, I ended up converting everything. And discovered a bug in the recipe engine param_name stuff. So CL out for that next week probably
oh. derp. I didn't actually read the change closely enough. That said, this won't actually override it, if that's what you want. I would just remove the override option entirely until something needs it, tbh.
Description was changed from ========== Allow bot_update callers to configure the rietveld server with python arguments. BUG= ========== to ========== bot_update: Rewrite to use properties, and add override options for patches. BUG= ==========
Description was changed from ========== bot_update: Rewrite to use properties, and add override options for patches. BUG= ========== to ========== bot_update: Rewrite to use properties, and add override options for patches. BUG=591172 ==========
PTAL Not sure I understand your comment; I need the ability to manually specify the rietveld server, in the call to bot_update.ensure_checkout....
Description was changed from ========== bot_update: Rewrite to use properties, and add override options for patches. BUG=591172 ========== to ========== bot_update: Rewrite to use properties, and add override options for patches. Depends on https://codereview.chromium.org/1755593003 BUG=591172 ==========
lgtm
your original change was
dict.get("key", DEFAULT_VALUE)
Which means that if 'key' is in dict (e.g. properties contains it),
DEFAULT_VALUE would be ignored. What you actually wanted (in the previous patch)
was
OVERRIDE_VALUE or dict.get("key")
https://codereview.chromium.org/1741983002/diff/20001/recipe_modules/bot_upda...
File recipe_modules/bot_update/__init__.py (right):
https://codereview.chromium.org/1741983002/diff/20001/recipe_modules/bot_upda...
recipe_modules/bot_update/__init__.py:16: PROPERTIES = {
huge regret time: DEPS should probably have been in api.py... mer.
https://codereview.chromium.org/1741983002/diff/20001/recipe_modules/bot_upda...
File recipe_modules/bot_update/api.py (right):
https://codereview.chromium.org/1741983002/diff/20001/recipe_modules/bot_upda...
recipe_modules/bot_update/api.py:69: self._properties = {}
can we rename this? like _last_returned_properties or something?
https://codereview.chromium.org/1741983002/diff/20001/recipe_modules/bot_upda...
recipe_modules/bot_update/api.py:100: properties.
mention which property for these. document the format of these properties up in
PROPERTIES if possible.
Yeah that's what I figured you meant. Fixed it! :) https://codereview.chromium.org/1741983002/diff/20001/recipe_modules/bot_upda... File recipe_modules/bot_update/api.py (right): https://codereview.chromium.org/1741983002/diff/20001/recipe_modules/bot_upda... recipe_modules/bot_update/api.py:69: self._properties = {} On 2016/03/01 at 22:36:22, iannucci wrote: > can we rename this? like _last_returned_properties or something? Done. Might have to rename callers though, I'll see. https://codereview.chromium.org/1741983002/diff/20001/recipe_modules/bot_upda... recipe_modules/bot_update/api.py:100: properties. On 2016/03/01 at 22:36:22, iannucci wrote: > mention which property for these. document the format of these properties up in PROPERTIES if possible. I mentioned which property these refer to. What do you mean document the format?
martiniss@chromium.org changed reviewers: + hinoka@chromium.org
PTAL hinoka. Are you ok with the modification to the ensure_checkout interface? A bit hacky; this works, but might not be the best thing to do?
Oh I forgot to ask: why does the server URL need to be overridden? On Tue, Mar 1, 2016, 17:23 <martiniss@chromium.org> wrote: > PTAL hinoka. Are you ok with the modification to the ensure_checkout > interface? > A bit hacky; this works, but might not be the best thing to do? > > https://codereview.chromium.org/1741983002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
hinoka@google.com changed reviewers: + hinoka@google.com
lg tho, assuming you trained the recipes and theres no expectation change. https://codereview.chromium.org/1741983002/diff/40001/recipe_modules/bot_upda... File recipe_modules/bot_update/__init__.py (right): https://codereview.chromium.org/1741983002/diff/40001/recipe_modules/bot_upda... recipe_modules/bot_update/__init__.py:16: PROPERTIES = { I don't think I understand this very well.. how does this end up in the BotUpdateApi class?? https://codereview.chromium.org/1741983002/diff/40001/recipe_modules/bot_upda... File recipe_modules/bot_update/api.py (right): https://codereview.chromium.org/1741983002/diff/40001/recipe_modules/bot_upda... recipe_modules/bot_update/api.py:97: run_hooks: Whether or not to run `gclient runhooks` after we checkout the Is this used anywhere?
On 2016/03/02 at 02:09:34, hinoka wrote: > lg tho, assuming you trained the recipes and theres no expectation change. > > https://codereview.chromium.org/1741983002/diff/40001/recipe_modules/bot_upda... > File recipe_modules/bot_update/__init__.py (right): > > https://codereview.chromium.org/1741983002/diff/40001/recipe_modules/bot_upda... > recipe_modules/bot_update/__init__.py:16: PROPERTIES = { > I don't think I understand this very well.. how does this end up in the BotUpdateApi class?? > > https://codereview.chromium.org/1741983002/diff/40001/recipe_modules/bot_upda... > File recipe_modules/bot_update/api.py (right): > > https://codereview.chromium.org/1741983002/diff/40001/recipe_modules/bot_upda... > recipe_modules/bot_update/api.py:97: run_hooks: Whether or not to run `gclient runhooks` after we checkout the > Is this used anywhere? What are we waiting for on this CL? There are other CLs that are going to land soon and force a rebase/rewrite here?
martiniss@chromium.org changed reviewers: + szager@chromium.org
FYI stefan, moved your change into this one. Going to land this later today.
https://codereview.chromium.org/1741983002/diff/100001/recipe_modules/bot_upd... File recipe_modules/bot_update/api.py (right): https://codereview.chromium.org/1741983002/diff/100001/recipe_modules/bot_upd... recipe_modules/bot_update/api.py:51: def __init__(self, mastername, buildername, slavename, issue, patchset, You added mandatory parameters, but did not change instantiations? This is dark magic. https://codereview.chromium.org/1741983002/diff/100001/recipe_modules/bot_upd... recipe_modules/bot_update/api.py:93: root_solution_revision=None, rietveld=None, issue=None, 17 named parameters is really not very many. Can you 8 or 9 more? https://codereview.chromium.org/1741983002/diff/100001/recipe_modules/bot_upd... recipe_modules/bot_update/api.py:105: git_cache_dir: The directory to use as the git cache. If omitted, will use This isn't actually an arg.
https://codereview.chromium.org/1741983002/diff/100001/recipe_modules/bot_upd... File recipe_modules/bot_update/api.py (right): https://codereview.chromium.org/1741983002/diff/100001/recipe_modules/bot_upd... recipe_modules/bot_update/api.py:51: def __init__(self, mastername, buildername, slavename, issue, patchset, On 2016/03/10 at 21:28:16, szager1 wrote: > You added mandatory parameters, but did not change instantiations? This is dark magic. So this dark magic is so that people don't do raw api.m.properties['foobar'] dark magic. So we're roughly replacing one dark magic to another. And, no use actually instantiates this class. It's created by the recipe engine code. https://codereview.chromium.org/1741983002/diff/100001/recipe_modules/bot_upd... recipe_modules/bot_update/api.py:93: root_solution_revision=None, rietveld=None, issue=None, On 2016/03/10 at 21:28:16, szager1 wrote: > 17 named parameters is really not very many. Can you 8 or 9 more? :( I know. https://codereview.chromium.org/1741983002/diff/100001/recipe_modules/bot_upd... recipe_modules/bot_update/api.py:105: git_cache_dir: The directory to use as the git cache. If omitted, will use On 2016/03/10 at 21:28:16, szager1 wrote: > This isn't actually an arg. Whoops, removed this but forgot the docs. Thanks.
lgtm, though I feel like the properties change should be split out from the change to add more arguments. https://chromiumcodereview.appspot.com/1741983002/diff/100001/recipe_modules/... File recipe_modules/bot_update/__init__.py (right): https://chromiumcodereview.appspot.com/1741983002/diff/100001/recipe_modules/... recipe_modules/bot_update/__init__.py:30: 'deps_revision_overrides': Property(default={}), file a bug for this TODO, mention it in the comment https://chromiumcodereview.appspot.com/1741983002/diff/100001/recipe_modules/... File recipe_modules/bot_update/api.py (right): https://chromiumcodereview.appspot.com/1741983002/diff/100001/recipe_modules/... recipe_modules/bot_update/api.py:51: def __init__(self, mastername, buildername, slavename, issue, patchset, On 2016/03/10 at 21:28:16, szager1 wrote: > You added mandatory parameters, but did not change instantiations? This is dark magic. these are filled by the explicit properties mechanism. Instead of having modules implicitly change their behavior at runtime by probing api.properties, all of the potentially behavior changing properties are added up front. This constructor is only called by the engine, not user code. See the listing of properties in __init__.py. These reflect the current reality, they're not changing the behavior of this module. The number of these properties needs to be whittled down over time, but we can't fix everything in one CL. https://chromiumcodereview.appspot.com/1741983002/diff/100001/recipe_modules/... recipe_modules/bot_update/api.py:93: root_solution_revision=None, rietveld=None, issue=None, On 2016/03/10 at 21:28:16, szager1 wrote: > 17 named parameters is really not very many. Can you 8 or 9 more? This is not an actionable or helpful comment. It's well known that this API has too many parameters. The only reason that this is changing in this CL is because YOU added these additional parameters.
On 2016/03/10 at 21:43:12, iannucci wrote: > lgtm, though I feel like the properties change should be split out from the change to add more arguments. > > https://chromiumcodereview.appspot.com/1741983002/diff/100001/recipe_modules/... > File recipe_modules/bot_update/__init__.py (right): > > https://chromiumcodereview.appspot.com/1741983002/diff/100001/recipe_modules/... > recipe_modules/bot_update/__init__.py:30: 'deps_revision_overrides': Property(default={}), > file a bug for this TODO, mention it in the comment > > https://chromiumcodereview.appspot.com/1741983002/diff/100001/recipe_modules/... > File recipe_modules/bot_update/api.py (right): > > https://chromiumcodereview.appspot.com/1741983002/diff/100001/recipe_modules/... > recipe_modules/bot_update/api.py:51: def __init__(self, mastername, buildername, slavename, issue, patchset, > On 2016/03/10 at 21:28:16, szager1 wrote: > > You added mandatory parameters, but did not change instantiations? This is dark magic. > > these are filled by the explicit properties mechanism. Instead of having modules implicitly change their behavior at runtime by probing api.properties, all of the potentially behavior changing properties are added up front. This constructor is only called by the engine, not user code. See the listing of properties in __init__.py. These reflect the current reality, they're not changing the behavior of this module. > > The number of these properties needs to be whittled down over time, but we can't fix everything in one CL. > > https://chromiumcodereview.appspot.com/1741983002/diff/100001/recipe_modules/... > recipe_modules/bot_update/api.py:93: root_solution_revision=None, rietveld=None, issue=None, > On 2016/03/10 at 21:28:16, szager1 wrote: > > 17 named parameters is really not very many. Can you 8 or 9 more? > > This is not an actionable or helpful comment. It's well known that this API has too many parameters. The only reason that this is changing in this CL is because YOU added these additional parameters. Oops. I'm wrong >_< nevermind me... sorry about that :(
On 2016/03/10 at 21:48:26, iannucci wrote: > On 2016/03/10 at 21:43:12, iannucci wrote: > > lgtm, though I feel like the properties change should be split out from the change to add more arguments. > > > > https://chromiumcodereview.appspot.com/1741983002/diff/100001/recipe_modules/... > > File recipe_modules/bot_update/__init__.py (right): > > > > https://chromiumcodereview.appspot.com/1741983002/diff/100001/recipe_modules/... > > recipe_modules/bot_update/__init__.py:30: 'deps_revision_overrides': Property(default={}), > > file a bug for this TODO, mention it in the comment > > > > https://chromiumcodereview.appspot.com/1741983002/diff/100001/recipe_modules/... > > File recipe_modules/bot_update/api.py (right): > > > > https://chromiumcodereview.appspot.com/1741983002/diff/100001/recipe_modules/... > > recipe_modules/bot_update/api.py:51: def __init__(self, mastername, buildername, slavename, issue, patchset, > > On 2016/03/10 at 21:28:16, szager1 wrote: > > > You added mandatory parameters, but did not change instantiations? This is dark magic. > > > > these are filled by the explicit properties mechanism. Instead of having modules implicitly change their behavior at runtime by probing api.properties, all of the potentially behavior changing properties are added up front. This constructor is only called by the engine, not user code. See the listing of properties in __init__.py. These reflect the current reality, they're not changing the behavior of this module. > > > > The number of these properties needs to be whittled down over time, but we can't fix everything in one CL. > > > > https://chromiumcodereview.appspot.com/1741983002/diff/100001/recipe_modules/... > > recipe_modules/bot_update/api.py:93: root_solution_revision=None, rietveld=None, issue=None, > > On 2016/03/10 at 21:28:16, szager1 wrote: > > > 17 named parameters is really not very many. Can you 8 or 9 more? > > > > This is not an actionable or helpful comment. It's well known that this API has too many parameters. The only reason that this is changing in this CL is because YOU added these additional parameters. > > Oops. I'm wrong >_< nevermind me... sorry about that :( (but we should still cut this CL into logical bits)
On 2016/03/10 at 21:52:10, iannucci wrote: > On 2016/03/10 at 21:48:26, iannucci wrote: > > On 2016/03/10 at 21:43:12, iannucci wrote: > > > lgtm, though I feel like the properties change should be split out from the change to add more arguments. > > > > > > https://chromiumcodereview.appspot.com/1741983002/diff/100001/recipe_modules/... > > > File recipe_modules/bot_update/__init__.py (right): > > > > > > https://chromiumcodereview.appspot.com/1741983002/diff/100001/recipe_modules/... > > > recipe_modules/bot_update/__init__.py:30: 'deps_revision_overrides': Property(default={}), > > > file a bug for this TODO, mention it in the comment > > > > > > https://chromiumcodereview.appspot.com/1741983002/diff/100001/recipe_modules/... > > > File recipe_modules/bot_update/api.py (right): > > > > > > https://chromiumcodereview.appspot.com/1741983002/diff/100001/recipe_modules/... > > > recipe_modules/bot_update/api.py:51: def __init__(self, mastername, buildername, slavename, issue, patchset, > > > On 2016/03/10 at 21:28:16, szager1 wrote: > > > > You added mandatory parameters, but did not change instantiations? This is dark magic. > > > > > > these are filled by the explicit properties mechanism. Instead of having modules implicitly change their behavior at runtime by probing api.properties, all of the potentially behavior changing properties are added up front. This constructor is only called by the engine, not user code. See the listing of properties in __init__.py. These reflect the current reality, they're not changing the behavior of this module. > > > > > > The number of these properties needs to be whittled down over time, but we can't fix everything in one CL. > > > > > > https://chromiumcodereview.appspot.com/1741983002/diff/100001/recipe_modules/... > > > recipe_modules/bot_update/api.py:93: root_solution_revision=None, rietveld=None, issue=None, > > > On 2016/03/10 at 21:28:16, szager1 wrote: > > > > 17 named parameters is really not very many. Can you 8 or 9 more? > > > > > > This is not an actionable or helpful comment. It's well known that this API has too many parameters. The only reason that this is changing in this CL is because YOU added these additional parameters. > > > > Oops. I'm wrong >_< nevermind me... sorry about that :( > > (but we should still cut this CL into logical bits) ((and stop adding parameters in general...))
The CQ bit was checked by martiniss@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from iannucci@chromium.org Link to the patchset: https://codereview.chromium.org/1741983002/#ps160001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1741983002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1741983002/160001
Message was sent while issue was closed.
Description was changed from ========== bot_update: Rewrite to use properties, and add override options for patches. Depends on https://codereview.chromium.org/1755593003 BUG=591172 ========== to ========== bot_update: Rewrite to use properties, and add override options for patches. Depends on https://codereview.chromium.org/1755593003 BUG=591172 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=299332 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as http://src.chromium.org/viewvc/chrome?view=rev&revision=299332 |
