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

[Dustin J. Mitchell]

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?

---

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.

===

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.

===

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