Backward compatible renaming of "slave"-named objects in master-part

[rutsky]

---

This is not POC anymore, but WIP branch of renaming "slave"-named objects in master-part of Buildbot

---

This is same PR as I done in my repo (rutsky#1), but cherry-picked against current Buildbot master branch.

This is Proof-of-Concept of backward-compatible renaming of "slave"-named classes/methods/attributes to "worker" for the "renaming" project (Trac ticket, my application letter with details and related discussion).

This POC renames some interfaces/methods/attributes according to cases described below.
Some code uses renamed interfaces/methods and most code (in tests) uses old interface.
All tests are passing, and you can see a lot of Use of obsolete name warnings, so this approach looks promising.

Some thought and notes about this POC code.

Following cases of old API usage that have slave in it should be considered:

1. Import of old name:

from buildbot.interfaces import IBuildSlave
from buildbot.interfaces import NoSlaveError

Warning can be issued if module (in sys.modules) will be replaced with wrapper class with overloaded __getattr__ — this is a terrible and not reliable hack. I don't think it's feasible to use it.

2. Use of old name (without instantiation):

class X(IBuildSlave):
     pass

class Y:
    zope.interface.implements(IBuildSlave)

try:
    ...
except NoSlaveError:

No chance?

3. Wildcard import:

from buildbot.interfaces import *

No chance?

4. Old-name classes that may be instantiated:

https://github.com/rutsky/buildbot/pull/1/files#diff-fe204dcb39a98c86b49ad9abbce9921bR152

5. Old-named free functions.

Easy, but not yet implemented.

6. Old-named methods (is @defer.inlineCallbacks adds complexity here?):

https://github.com/rutsky/buildbot/pull/1/files#diff-c72eed05317a89a2f636d49f3f323341R333

7. Old-named attributes.

https://github.com/rutsky/buildbot/pull/1/files#diff-5629977527acc490d9536a5a154865b1R144

8. Old-named properties.

https://github.com/rutsky/buildbot/pull/1/files#diff-c72eed05317a89a2f636d49f3f323341R132

9. Modules.

Not considered yet. Looks like simplest approach to rename current, e.g. buildbot.buildslave to buildbot.worker and add dummy module (and all submodules) buildbot.buildslave:

# buildbot.buildslave

# <print warning here>

from buildbot.worker import *

10. Assignment to old API name from the outside:

master = BuildMaster(...)
master.buildslaves = ...

Handled in attributes code. But when old API will be completely removed these cases may lead to problems.

11. Backward compatible keyword argument names + renderables:

class FileDownload(_TransferBuildStep):

    name = 'download'

    renderables = ['mastersrc', 'slavedest']

    def __init__(self, mastersrc, slavedest,

I believe renderables can be not backward compatible renamed, since UI is changed completely.

Keyword arguments may be handled like this:

    def __init__(self, mastersrc, workerdest=None, 
            slavedest=None, # obsolete
            ):
        assert workerdest is not None != slavedest is not None, "Only one argument should be specified: ..."
        if slavedest is not None:
            workerdest = slavedest
        # use only `workerdest` below.

Few more (TODO) notes:

1. Log warning only once per attribute/class usage, e.g. not for every attribute access.
   Dustin suggested to take a look at Twisted utilities for deprecation warnings, or warnings Python module.
2. Is somewhere objects are introspected? E.g. pickle, iterations over __dict__, iterations over supported interfaces? This may create problems.

Dustin said, that pickle-serialized stuff is dropped in nine.

Luckily there is no:

1. Old-named zope.interface.Attribute.

[rutsky]

Comments regarding this way of doing backward-compatible renaming in "renaming" project are welcome.

[codecov-io]

Current coverage is 84.17%

Merging #1943 into master will decrease coverage by -0.40% as of 3bf262d

@@            master   #1943   diff @@
======================================
  Files          320     330    +10
  Stmts        31151   31985   +834
  Branches         0       0       
  Methods          0       0       
======================================
+ Hit          26347   26924   +577
  Partial          0       0       
- Missed        4804    5061   +257

Review entire Coverage Diff as of 3bf262d

---

Uncovered Suggestions

1. +0.11% via ...ot/status/builder.py#381...414
2. +0.09% via ...ot/status/builder.py#453...481
3. +0.09% via ...dbot/changes/mail.py#478...505
4. See 7 more...

Powered by Codecov. Updated on successful CI builds.

[tardyp]

1. Import of old name:

I would go for a decorator on the old name
IBuildSlave = worker_transition.deprecated(IWorker)
Importing wont warn, but use will.
This means however that disabling the warns in c['..'] will not work, as it will be taken in account too late.

I think the best is to warn from the config. If in the config we see that the user used deprecated names, then we warn. It means that the deprecated decorator will return a function which sets a __used_deprecated_interface attribute after instanciating the new class.

Like I said in the other PR, its much easier for review if you look at the tests rather than the implementation. With tests we know clearly how the code is supposed to behave, and then we can more easily reason on it.

Lets continue to discuss (and test) on this PR, and we will rebase the pycharm generated refactor later

[rutsky]

@djmitche I switched to warnings module as you suggested --- now fallback behavior (print warning, ignore, raise error) can be configured in the same way as for other Python warnings:

import warnings
from buildbot.worker_transition import DeprecatedWorkerNameError
# Treat old-name usage as errors
warnings.simplefilter('error', DeprecatedWorkerNameError)

Code like above can be placed to buildbot.tac to treat warnings as errors in master configuration.

Plus out of the box place of where warning was issued is registered, and warning should be issued only once (if so configured in warnings module): https://travis-ci.org/buildbot/buildbot/jobs/100493402#L379.

@tardyp I wrote tests for wrapper functions: https://github.com/rutsky/buildbot/blob/worker_API_rename_POC2/master/buildbot/test/unit/test_worker_transition.py

@tardyp I don't get it, how you suggest to use __used_deprecated_interface or similar?
For example, IBuildSlave alias is never instantiated and used only with zope.interface's implements() or similar methods, I haven't found a way to hook them without patching.

For creating aliases I suggest to use something like

class IWorker(IPlugin):
    pass
# defines "IBuildSlave" in globals().
define_old_worker_class_alias(globals(), IWorker, pattern="BuildWorker")

instead of

class IWorker(IPlugin):
    pass
IBuildSlave = worker_transition.deprecated(IWorker)

to reduce number of places where "slave" string is used; plus, in most cases "worker"->"slave" rename can be done automatically and I implement it in _compat_name(): https://github.com/buildbot/buildbot/pull/1943/files#diff-4aca98f49bee441088b276fde1cd880fR43

[djmitche]

The warnings change sounds great!

Note that trial has some support for asserting that warnings were issued, if that helps in testing.

[rutsky]

Changes today:

• Renamed buildbot.buildslave.* to buildbot.worker.* and added fake buildbot.buildslave.* modules that just import names from copied modules: https://github.com/rutsky/buildbot/blob/worker_API_rename_POC2/master/buildbot/buildslave/__init__.py. All tests are still passing.
• Started writing documentation for API changes: https://github.com/rutsky/buildbot/blob/worker_API_rename_POC2/master/docs/manual/worker-transition.rst
• Added some tests for loading current version of configuration (soon stuff in configuration will be renamed).
• Replaced pyflakes with flake8 to quiet pyflakes warnings about star imports. How about replacing pep8 + pyflakes with flake8? Looks like flake8 does everything that pep8 and pyflake does, but also has additional options to disable warnings (e.g. # noqa). I don't see a way to use star import with pyflakes.
• Added warnings configuration to buildbot.tac template (which most probably will be removed from there, see TODO there).

[rutsky]

Today's changes:

• More classes were renamed in backward-compatible way.
• Use flake8 in validate.sh (may be considered as part of this ticket).
• Plugins infrastructure were updated to use buildbot.worker entry point and buildbot.plugins.worker namespace instead of buildslave-versions (in backward-compatible way of course). List of assumptions that were used during implementation is available here. Updated documentation is here. Expected behavior is tested.

[rutsky]

Today's changes:

• c['slaves'] is properly renamed to c['workers']. Fallback is documented and tested.
• Warnings that are expected during configurations loading are explicitly checked.
• Warnings helper functions refactored and tested.
• Usage of previously renamed classes/attributes are properly propagated for all master source code.
• Most of slave wording inside comments were updated.

Now master tests don't show bunch of DeprecatedWorkerAPIWarning warnings: warnings from usage of renamed interface inside Buildbot were fixed, and warnings from tests of loading old configuration files are explicitly catched and verified.

[djmitche]

This sounds great! Are there any particular questions you'd like me to ponder? Anything else you need from me? Dustin

[rutsky]

@djmitche not now, yet. It's more like I'm reporting my progress here.

I will prepare questions for the next meeting. There are bunch of them — I wrote them in the code as TODO notes, so they need to be resolved before merging. Most of them don't require immediate attention.

[rutsky]

Recent changes:

• Most of public API is updated (with providing appropriate fallback for old names).
• Migration for database were added (works in SQLite and MySQL, but in PostgreSQL Buildbot tests fails: http://trac.buildbot.net/ticket/3419).
• Endpoints in Data API were updated along with object contents.
• Web UI were updated (both base and md_base). BTW @tardyp md_base is looking pretty good indeed!

Only one "big" part is left: docs update.
Plus a lot of minor local renamings mostly in tests; perhaps few more API changes (e.g. I haven't updated MQ level yet).

@djmitche, @tardyp, from this point review of this PR can be started.
At first, please review API changes: https://github.com/rutsky/buildbot/blob/worker_API_rename_POC2/master/docs/manual/worker-transition.rst

1. Maybe some API is private and don't require fallback for old names?
2. Maybe some (as I thought) private API is widely used and fallback is required?
3. Maybe you don't like some names?

Other review instructions (e.g. how to review fallback/compatibility for old names implementation) I will provide later.

[rutsky]

Further development will be done in bug2340 branch. Closing this PR.