<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 21.02.2016 00:16, Pierre Tardy
wrote:<br>
</div>
<blockquote
cite="mid:CAJ+soVdzs2H20BVUk1tWBMaawNg+06KbOs-DBX5DxwXkype+Cg@mail.gmail.com"
type="cite">
<div dir="ltr"><br>
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
- [ ] To get raw diff via git:<br>
<br>
git clone <a moz-do-not-send="true"
href="https://github.com/buildbot/buildbot.git"
rel="noreferrer" target="_blank">https://github.com/buildbot/buildbot.git</a><br>
cd buildbot<br>
git fetch --all<br>
git diff origin/master...origin/bug2340<br>
</blockquote>
<div><br>
</div>
<div>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.</div>
<div>
<div>
<div><br>
</div>
<div><br>
</div>
<div>git diff buildbot/master
master/docs/manual/worker-transition.rst</div>
</div>
<div><br>
</div>
<div>+ ``workdir`` property source from ``"slave
(deprecated)"`` to</div>
<div>+ ``"worker (deprecated)"``.</div>
</div>
<div>While we are at it, I would remove those deprecated
properties. They have been for a good while, and deserve a
fair death.</div>
</div>
</div>
</blockquote>
<br>
+1 for removing them (added TODO for this one).<br>
<br>
<blockquote
cite="mid:CAJ+soVdzs2H20BVUk1tWBMaawNg+06KbOs-DBX5DxwXkype+Cg@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_quote">
<div>
<div>+API changes between 0.9.0b7 and 0.9.0b8 (done without
providing fallback).</div>
<div>+</div>
<div>+.. todo::</div>
<div>+</div>
<div>+ This whole section may be removed since it's not
important for users</div>
<div>+ upgrading to 0.9.0.</div>
<div>+</div>
</div>
<div><br>
</div>
<div>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.</div>
</div>
</div>
</blockquote>
<br>
As we discussed with Dustin, I'll move changes that done without
fallback <br>
(i.e. introduced in nine branch) from worker_transition.rst into
release notes<br>
of the next beta.<br>
<br>
<blockquote
cite="mid:CAJ+soVdzs2H20BVUk1tWBMaawNg+06KbOs-DBX5DxwXkype+Cg@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_quote">
<div><br>
</div>
<div>
<div>+ - ``/masters/n:masterid/workers/n:workersid``</div>
</div>
<div><br>
</div>
<div>its workerid. I can verify that the code is okay, this
is just a typo in the doc.<br>
</div>
</div>
</div>
</blockquote>
<br>
Oops! Fixed: <a
href="https://github.com/buildbot/buildbot/commit/05a599ee207011e81f9e7c1bc9aac73be876ea0f"><a class="moz-txt-link-freetext" href="https://github.com/buildbot/buildbot/commit/05a599ee207011e81f9e7c1bc9aac73be876ea0f">https://github.com/buildbot/buildbot/commit/05a599ee207011e81f9e7c1bc9aac73be876ea0f</a></a><br>
<br>
<blockquote
cite="mid:CAJ+soVdzs2H20BVUk1tWBMaawNg+06KbOs-DBX5DxwXkype+Cg@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_quote">
<div> </div>
<div>
<div> git diff buildbot/master
master/buildbot/test/unit/test_config.py</div>
</div>
<div><br>
</div>
<div>
<div>+from buildbot.test.util.warnings import
assertNotProducesWarnings</div>
<div>+from buildbot.test.util.warnings import
assertProducesWarning</div>
</div>
<div>[...]</div>
<div>
<div>+ with assertProducesWarning(</div>
</div>
<div>[nit] I think it is more unittest idiomatic to say 'with
self.assertProducesWarning', means you would rather import
something like:</div>
<div>
<div>from buildbot.test.util.warnings import
warningAssertionMixins</div>
</div>
</div>
</div>
</blockquote>
<br>
Yeah, I thought about that initially. I decided to make it in this
way<br>
because these helpers don't strongly depend on `self` (I wasn't <br>
sure about context where they will be used), plus it's <br>
easier to convert them from current implementation into <br>
`warningAssertionMixins` if it would be needed, rather than in
reverse.<br>
<br>
Looking at they usage now I agree with you, Pierre:<br>
<br>
1. There is no point to use them outside of tests.<br>
<br>
2. They actually depend on `self`: they can do
`self.assertEqual`/`self.assertTrue`<br>
instead of simply `assert`.<br>
<br>
Added TODO for this.<br>
<br>
<blockquote
cite="mid:CAJ+soVdzs2H20BVUk1tWBMaawNg+06KbOs-DBX5DxwXkype+Cg@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_quote">
<div><br>
</div>
<div><br>
</div>
<div>
<div>diff --git a/master/buildbot/db/buildslave.py
b/master/buildbot/db/buildslave.py</div>
<div>new file mode 100644</div>
<div>index 0000000..4f486b1</div>
<div>--- /dev/null</div>
<div>+++ b/master/buildbot/db/buildslave.py</div>
<div>@@ -0,0 +1,30 @@</div>
<div>[...]</div>
<div>+deprecatedWorkerModuleAttribute(locals(),
_WorkersConnectorComponent,<br>
</div>
<div>+
compat_name="BuildslavesConnectorComponent",</div>
<div>+
new_name="WorkersConnectorComponent")</div>
<div>diff --git a/master/buildbot/db/buildslaves.py
b/master/buildbot/db/buildslaves.py</div>
<div>deleted file mode 100644</div>
<div>index 8613f03..0000000</div>
<div>--- a/master/buildbot/db/buildslaves.py</div>
<div>+++ /dev/null</div>
</div>
<div><br>
</div>
<div>looks like db/buildslaves.py was renamed in the process
of deprecation!</div>
</div>
</div>
</blockquote>
<br>
Good catch!<br>
Fixed: <a
href="https://github.com/buildbot/buildbot/commit/bc26163853fd61d79742696f98b79ef526b39329"><a class="moz-txt-link-freetext" href="https://github.com/buildbot/buildbot/commit/bc26163853fd61d79742696f98b79ef526b39329">https://github.com/buildbot/buildbot/commit/bc26163853fd61d79742696f98b79ef526b39329</a></a><br>
<br>
<blockquote
cite="mid:CAJ+soVdzs2H20BVUk1tWBMaawNg+06KbOs-DBX5DxwXkype+Cg@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_quote">
<div><br>
</div>
<div><br>
</div>
<div>I think this whole thing is a great job! I join Dustin on
the amazin' words.</div>
<div><br>
</div>
<div>As we know this beast is working (<a
moz-do-not-send="true" href="http://nine.bb.net">nine.bb.net</a>),
IMHO we should merge it and treat the remaining todos in
subsequent PR, which will be much much easier to review.</div>
<div><br>
</div>
<div>Cheers</div>
<div>Pierre</div>
</div>
</div>
</blockquote>
<br>
I moved TODO notes from PR and your notes into bug description (<a
href="http://trac.buildbot.net/ticket/2340"><a class="moz-txt-link-freetext" href="http://trac.buildbot.net/ticket/2340">http://trac.buildbot.net/ticket/2340</a></a>).<br>
<br>
Lets merge this PR and I'll provide separate PRs for remaining
issues<br>
(plus large part with `buildbot-slave` -> `buildbot-worker`
rename)!<br>
<br>
Thanks for review, Pierre!<br>
<br>
--<br>
Vladimir<br>
<br>
</body>
</html>