<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">On 17.02.2016 03:52, Dustin J. Mitchell
wrote:<br>
</div>
<blockquote
cite="mid:CAJtE5vR_zk7bJor+psOFmvmFfsn4+TCXy2igSc52aF8DaMs_5g@mail.gmail.com"
type="cite">
<pre wrap="">I had a look at this. I've also been following along pretty closely,
so a lot of this isn't new to me. This is some great code, and a
really solid and well-organized approach to the problem.
I would love to get sa2ajj's input in particular about plugins, and
another set of eyes from mine would be helpful for such a large-scale
change as this. So please don't take my review as indicating this
doesn't need your attention!
Dustin
===
DB migration looks great
===
Old names fallback support
Nicely done; minor comments:
<a class="moz-txt-link-freetext" href="https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/worker_transition.py#L56">https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/worker_transition.py#L56</a>
If `name` is not specified old name is construct by replacing:
"worker" -> "slave",
"Worker" -> "Slave".
I think you mean `compat_name` here?</pre>
</blockquote>
<br>
Yes, I missed this place during refactoring.<br>
Fixed: <a
href="https://github.com/buildbot/buildbot/commit/a578f094899afa944d94b561caeca8cd4614daca"><a class="moz-txt-link-freetext" href="https://github.com/buildbot/buildbot/commit/a578f094899afa944d94b561caeca8cd4614daca">https://github.com/buildbot/buildbot/commit/a578f094899afa944d94b561caeca8cd4614daca</a></a><br>
<br>
<blockquote
cite="mid:CAJtE5vR_zk7bJor+psOFmvmFfsn4+TCXy2igSc52aF8DaMs_5g@mail.gmail.com"
type="cite">
<pre wrap="">
---
<a class="moz-txt-link-freetext" href="https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/worker_transition.py#L199">https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/worker_transition.py#L199</a>
attribute_name = scope.keys()[scope.values().index(attribute)]
Does Python guarantee stable ordering of values() and keys()? If not,
I suspect there are better one-liners for this inverted lookup.</pre>
</blockquote>
<br>
Yes, Python guarantees order of keys()/values():<br>
<br>
> If items(), keys(), values(), iteritems(), iterkeys(), and
itervalues() are called with no intervening modifications to the
dictionary, the lists will directly correspond.<br>
(<a
href="https://docs.python.org/2.7/library/stdtypes.html#dict.items"><a class="moz-txt-link-freetext" href="https://docs.python.org/2.7/library/stdtypes.html#dict.items">https://docs.python.org/2.7/library/stdtypes.html#dict.items</a></a>)<br>
<br>
<blockquote
cite="mid:CAJtE5vR_zk7bJor+psOFmvmFfsn4+TCXy2igSc52aF8DaMs_5g@mail.gmail.com"
type="cite">
<pre wrap="">
===
Warnings catching helpers
Also nicely done
===
Fallbacks support in Plugins
Looks good - I'd like to hear from sa2ajj, who may also be able to
answer the TODO question in the tests
===
Fallback for properties
This is really elegant. Yes, Properties, Interpolate, and
WithProperties are the three cases I know of where properties are
read.</pre>
</blockquote>
<br>
OK, then I'll remove that TODO note ("
<meta http-equiv="content-type" content="text/html; charset=utf-8">
TODO: Is all cases of property usage handled?").<br>
<br>
<br>
Thanks, Dustin!<br>
<br>
--<br>
Vladimir<br>
<br>
<br>
<blockquote
cite="mid:CAJtE5vR_zk7bJor+psOFmvmFfsn4+TCXy2igSc52aF8DaMs_5g@mail.gmail.com"
type="cite">
<pre wrap="">
===
Dustin
On Mon, Feb 15, 2016 at 5:25 AM, Vladimir Rutsky
<a class="moz-txt-link-rfc2396E" href="mailto:rutsky.vladimir@gmail.com"><rutsky.vladimir@gmail.com></a> wrote:
</pre>
<blockquote type="cite">
<pre wrap="">Hello!
I finished main parts of renaming in master and asking to review my changes.
Few more little changes are expected to land in that branch, but they can
be applied and after bug2340 branch merging.
Main goal of this branch is to rename all "slave" to "worker" in master part
of
Buildbot (i.e. buildslave module and communication with it from master are
not touched). API that is available in eight branch is changed in backward
compatible way, API that were introduced in nine branch renamed without
providing fallback.
Buildbot should be completely function in bug2340 branch, changes in master
branch are merged into it.
Actual list of topics to take a look at available in PR description:
<a class="moz-txt-link-freetext" href="https://github.com/buildbot/buildbot/pull/1962">https://github.com/buildbot/buildbot/pull/1962</a>
Review high-level API changes
-----
- [x] PR with high-level changes for review:
<a class="moz-txt-link-freetext" href="https://github.com/buildbot/buildbot/pull/1961">https://github.com/buildbot/buildbot/pull/1961</a>
(done by Dustin)
Review Database schema migration
-----
- [ ]
<a class="moz-txt-link-freetext" href="https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/db/migrate/versions/045_worker_transition.py">https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/db/migrate/versions/045_worker_transition.py</a>
<a class="moz-txt-link-freetext" href="https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/test/unit/test_db_migrate_versions_045_worker_transition.py">https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/test/unit/test_db_migrate_versions_045_worker_transition.py</a>
<a class="moz-txt-link-freetext" href="https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/db/model.py">https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/db/model.py</a>
This also fixes <a class="moz-txt-link-freetext" href="http://trac.buildbot.net/ticket/3088">http://trac.buildbot.net/ticket/3088</a>.
Review old names fallback support
-----
- [ ] Methods for providing fallbacks implemented here:
<a class="moz-txt-link-freetext" href="https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/worker_transition.py">https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/worker_transition.py</a>
Tests for them:
<a class="moz-txt-link-freetext" href="https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/test/unit/test_worker_transition.py">https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/test/unit/test_worker_transition.py</a>
Review warnings catching helpers
-----
- [ ] Warnings catching helpers:
<a class="moz-txt-link-freetext" href="https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/test/util/warnings.py">https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/test/util/warnings.py</a>
Tests:
<a class="moz-txt-link-freetext" href="https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/test/unit/test_test_util_warnings.py">https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/test/unit/test_test_util_warnings.py</a>
Review fallbacks support in Plugins
-----
- [ ]
<a class="moz-txt-link-freetext" href="https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/plugins/db.py#L365">https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/plugins/db.py#L365</a>
<a class="moz-txt-link-freetext" href="https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/plugins/__init__.py#L42">https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/plugins/__init__.py#L42</a>
Tests:
<a class="moz-txt-link-freetext" href="https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/test/unit/test_plugins.py#L284">https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/test/unit/test_plugins.py#L284</a>
Review fallback for properties
-----
- [ ] Take a look at
<a class="moz-txt-link-freetext" href="https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/process/properties.py#L429">https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/process/properties.py#L429</a>
and check usage of `_on_property_usage()` in this module.
Tests:
<a class="moz-txt-link-freetext" href="https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/test/unit/test_process_properties.py#L1521">https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/test/unit/test_process_properties.py#L1521</a>
Review raw diff
-----
- [ ] To get raw diff via git:
git clone <a class="moz-txt-link-freetext" href="https://github.com/buildbot/buildbot.git">https://github.com/buildbot/buildbot.git</a>
cd buildbot
git fetch --all
git diff origin/master...origin/bug2340
Regards,
Vladimir Rutsky
_______________________________________________
devel mailing list
<a class="moz-txt-link-abbreviated" href="mailto:devel@buildbot.net">devel@buildbot.net</a>
<a class="moz-txt-link-freetext" href="https://lists.buildbot.net/mailman/listinfo/devel">https://lists.buildbot.net/mailman/listinfo/devel</a>
</pre>
</blockquote>
</blockquote>
<br>
</body>
</html>