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

[Vladimir Rutsky]

On 21.02.2016 00:16, Pierre Tardy wrote:
>
>
>     - [ ] 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
>
>
> I'll go for that one. This reminds me the good old time when I 
> reviewed the nine branch from dustin, trying to understand what all 
> this data api thing was about.
>
>
> git diff buildbot/master master/docs/manual/worker-transition.rst
>
> +  ``workdir`` property source from ``"slave (deprecated)"`` to
> +  ``"worker (deprecated)"``.
> While we are at it, I would remove those deprecated properties. They 
> have been for a good while, and deserve a fair death.

+1 for removing them (added TODO for this one).

> +API changes between 0.9.0b7 and 0.9.0b8 (done without providing 
> fallback).
> +
> +.. todo::
> +
> +   This whole section may be removed since it's not important for users
> +   upgrading to 0.9.0.
> +
>
> Well, there as been already some people adopting nine before it was 
> released (at least people in my extended team), so I think it should 
> be good to keep it at least for a little time.

As we discussed with Dustin, I'll move changes that done without fallback
(i.e. introduced in nine branch) from worker_transition.rst into release 
notes
of the next beta.

>
> +     - ``/masters/n:masterid/workers/n:workersid``
>
> its  workerid. I can verify that the code is okay, this is just a typo 
> in the doc.

Oops! Fixed: 
https://github.com/buildbot/buildbot/commit/05a599ee207011e81f9e7c1bc9aac73be876ea0f

>  git diff buildbot/master master/buildbot/test/unit/test_config.py
>
> +from buildbot.test.util.warnings import assertNotProducesWarnings
> +from buildbot.test.util.warnings import assertProducesWarning
> [...]
> +        with assertProducesWarning(
> [nit] I think it is more unittest idiomatic to say 'with 
> self.assertProducesWarning', means you would rather import something like:
> from buildbot.test.util.warnings import warningAssertionMixins

Yeah, I thought about that initially. I decided to make it in this way
because these helpers don't strongly depend on `self` (I wasn't
sure about context where they will be used), plus it's
easier to convert them from current implementation into
`warningAssertionMixins` if it would be needed, rather than in reverse.

Looking at they usage now I agree with you, Pierre:

1. There is no point to use them outside of tests.

2. They actually depend on `self`: they can do 
`self.assertEqual`/`self.assertTrue`
instead of simply `assert`.

Added TODO for this.

>
>
> diff --git a/master/buildbot/db/buildslave.py 
> b/master/buildbot/db/buildslave.py
> new file mode 100644
> index 0000000..4f486b1
> --- /dev/null
> +++ b/master/buildbot/db/buildslave.py
> @@ -0,0 +1,30 @@
> [...]
> +deprecatedWorkerModuleAttribute(locals(), _WorkersConnectorComponent,
> +  compat_name="BuildslavesConnectorComponent",
> +  new_name="WorkersConnectorComponent")
> diff --git a/master/buildbot/db/buildslaves.py 
> b/master/buildbot/db/buildslaves.py
> deleted file mode 100644
> index 8613f03..0000000
> --- a/master/buildbot/db/buildslaves.py
> +++ /dev/null
>
> looks like db/buildslaves.py was renamed in the process of deprecation!

Good catch!
Fixed: 
https://github.com/buildbot/buildbot/commit/bc26163853fd61d79742696f98b79ef526b39329

>
>
> I think this whole thing is a great job! I join Dustin on the amazin' 
> words.
>
> As we know this beast is working (nine.bb.net <http://nine.bb.net>), 
> IMHO we should merge it and treat the remaining todos in subsequent 
> PR, which will be much much easier to review.
>
> Cheers
> Pierre

I moved TODO notes from PR and your notes into bug description 
(http://trac.buildbot.net/ticket/2340).

Lets merge this PR and I'll provide separate PRs for remaining issues
(plus large part with `buildbot-slave` -> `buildbot-worker` rename)!

Thanks for review, Pierre!

--
Vladimir

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.buildbot.net/pipermail/devel/attachments/20160221/45ae8c91/attachment.html>