[devel at bb.net] Review request for bug2340 branch ("slave"->"worker" renaming in master)

[Vladimir Rutsky]

On 17.02.2016 03:52, Dustin J. Mitchell wrote:
> 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:
>
> https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/worker_transition.py#L56
> If `name` is not specified old name is construct by replacing:
> "worker" -> "slave",
> "Worker" -> "Slave".
>
> I think you mean `compat_name` here?

Yes, I missed this place during refactoring.
Fixed: 
https://github.com/buildbot/buildbot/commit/a578f094899afa944d94b561caeca8cd4614daca

>
> ---
>
> https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/worker_transition.py#L199
> 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.

Yes, Python guarantees order of keys()/values():

 > If items(), keys(), values(), iteritems(), iterkeys(), and 
itervalues() are called with no intervening modifications to the 
dictionary, the lists will directly correspond.
(https://docs.python.org/2.7/library/stdtypes.html#dict.items)

>
> ===
>
> 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.

OK, then I'll remove that TODO note (" TODO: Is all cases of property 
usage handled?").


Thanks, Dustin!

--
Vladimir


>
> ===
>
> Dustin
>
>
> On Mon, Feb 15, 2016 at 5:25 AM, Vladimir Rutsky
> <rutsky.vladimir at gmail.com> wrote:
>> 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:
>> https://github.com/buildbot/buildbot/pull/1962
>>
>> Review high-level API changes
>> -----
>>
>> - [x] PR with high-level changes for review:
>> https://github.com/buildbot/buildbot/pull/1961
>> (done by Dustin)
>>
>> Review Database schema migration
>> -----
>>
>> - [ ]
>> 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/test/unit/test_db_migrate_versions_045_worker_transition.py
>> https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/db/model.py
>>
>>       This also fixes http://trac.buildbot.net/ticket/3088.
>>
>> Review old names fallback support
>> -----
>> - [ ] Methods for providing fallbacks implemented here:
>> https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/worker_transition.py
>>
>>        Tests for them:
>> https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/test/unit/test_worker_transition.py
>>
>> Review warnings catching helpers
>> -----
>>
>> - [ ] Warnings catching helpers:
>> https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/test/util/warnings.py
>>
>>        Tests:
>> https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/test/unit/test_test_util_warnings.py
>>
>> Review fallbacks support in Plugins
>> -----
>>
>> - [ ]
>> https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/plugins/db.py#L365
>> https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/plugins/__init__.py#L42
>>
>>        Tests:
>> https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/test/unit/test_plugins.py#L284
>>
>> Review fallback for properties
>> -----
>>
>> - [ ] Take a look at
>> https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/process/properties.py#L429
>> and check usage of `_on_property_usage()` in this module.
>>
>>        Tests:
>> https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/test/unit/test_process_properties.py#L1521
>>
>> Review raw diff
>> -----
>>
>> - [ ] To get raw diff via git:
>>
>> git clone https://github.com/buildbot/buildbot.git
>> cd buildbot
>> git fetch --all
>> git diff origin/master...origin/bug2340
>>
>>
>> Regards,
>>
>> Vladimir Rutsky
>>
>> _______________________________________________
>> devel mailing list
>> devel at buildbot.net
>> https://lists.buildbot.net/mailman/listinfo/devel

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.buildbot.net/pipermail/devel/attachments/20160220/36620efe/attachment.html>