Skip to content

Cleaner deprecation process with decorators#32068

Merged
cachedout merged 67 commits intosaltstack:2015.8from
isbm:isbm-deprecation-process
Apr 6, 2016
Merged

Cleaner deprecation process with decorators#32068
cachedout merged 67 commits intosaltstack:2015.8from
isbm:isbm-deprecation-process

Conversation

@isbm
Copy link
Contributor

@isbm isbm commented Mar 22, 2016

What does this PR do?

Allows to deprecate any function with just a decorator, following a simple usage process. The process consists of two decorators: is_deprecated and with_deprecated. For example, if there is a need to rewrite a function with a different output, but an old content is still required to keep around with the same function signature, do the following:

from salt.decorators import with_deprecated

@with_deprecated(globals(), "Boron")
def foo():
    """
    This is brand new function.
    """
    return {"data": "New content", "error": None}

def _foo():
    """
    This is an old function that does not know how to return an error code.
    """
    return "Old content"

Users, who are heavily using foo() function and still require an old output (because of the API etc) still can enjoy an old version of it. For this, they should add the following entry to the /etc/salt/<minion|master|proxy> config file:

use_deprecated:
  - mymodule.foo

In this case Master or Minion or Proxy will switch to the older version of mymodule.foo.

This PR also contains an example of deprecated state.uptime function with a complete rewrite of it that does the same, but is nicely parse-able when used via the API.

What issues does this PR fix or reference?

  • There is a need to deprecate a function, replacing its content but keeping its name and/or signature
  • Put to the log a warning that a deprecated function is in use
  • Roll-back to the deprecated version of the replaced function, where its name is the same, while content is different.
  • Block further usage of the deprecated function, when its EOL is reached but function is still there for various reasons.

Tests written?

Yes!

Bo Maryniuk added 30 commits March 21, 2016 21:37
@isbm
Copy link
Contributor Author

isbm commented Mar 22, 2016

@rallytime Could you please help me here with the "Incompatible Python 3 code found"? To me it seems to be pretty much compatible, if I am not missing anything... 😧

@rallytime
Copy link
Contributor

@isbm I know the test failures here are a little confusing, and we're in the process of moving to a new pylint runner, so actually the only errors you need to worry about for pylint are located here:
http://166.78.178.63:8080/job/PR/job/salt-pr-lint-n/430/violations/

@isbm
Copy link
Contributor Author

isbm commented Mar 24, 2016

@rallytime Oh, wonderful. I broke a test and didn't noticed. OK, fixed. But I still have no idea why W0106 is happening here — there is nothing like %s formatting... As well as I have no clue why this is happening only there and doesn't happen, say, here or here, which should be the same thing for PyLint.

@isbm
Copy link
Contributor Author

isbm commented Mar 24, 2016

Added:

  • A bugfix, where I missed to re-raise raised exception by the decorator.
  • A test case for the new version of status.uptime

@cachedout
Copy link
Contributor

Looks like we still have some test failures here. Could you take another look, please @isbm

@cachedout
Copy link
Contributor

Go Go Jenkins!

@cachedout
Copy link
Contributor

@isbm We still have test failures here and can't get this merged until they're cleaned up. How would you want to proceed on this one?

@isbm
Copy link
Contributor Author

isbm commented Apr 5, 2016

@cachedout Something strange is happening to GitHub. I've just pushed two more commits, but they never shows up here, although Git returns all up to date and synchronised. However, when I try to checkout my pull request, commits never shows up. Also I cannot open anymore PR, as my new branches are also not shown up here. Today GitHub was down, maybe something wrong with their infra. While this is ridiculous, I am copypasting here them as patches here:

From 4670819a8a0bd0328bf0c0136815814fdf774774 Mon Sep 17 00:00:00 2001
From: Bo Maryniuk <bo@suse.de>
Date: Tue, 5 Apr 2016 23:21:15 +0200
Subject: [PATCH 1/2] Fix documentation

---
 salt/modules/status.py | 2 --
 1 file changed, 2 deletions(-)

diff --git a/salt/modules/status.py b/salt/modules/status.py
index 0d351b4..04c6204 100644
--- a/salt/modules/status.py
+++ b/salt/modules/status.py
@@ -135,8 +135,6 @@ def uptime():
     .. code-block:: bash

         salt '*' status.uptime
-
-    :return: A structure of the uptime
     '''
     ut_path = "/proc/uptime"
     if not os.path.exists(ut_path):
-- 
2.8.0

and:

From b2f1876b05c496a5d12f3d8144aac67122eeafeb Mon Sep 17 00:00:00 2001
From: Bo Maryniuk <bo@suse.de>
Date: Tue, 5 Apr 2016 23:21:39 +0200
Subject: [PATCH] Bugfix: proxy-pass the docstring of the decorated function

---
 salt/utils/decorators/__init__.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/salt/utils/decorators/__init__.py b/salt/utils/decorators/__init__.py
index 227b877..3b43504 100644
--- a/salt/utils/decorators/__init__.py
+++ b/salt/utils/decorators/__init__.py
@@ -578,6 +578,7 @@ class _WithDeprecated(_DeprecationDecorator):
                     raise CommandExecutionError(' '.join(msg))
             return self._call_function(kwargs)

+        _decorate.__doc__ = self._function.__doc__
         return _decorate


-- 
2.8.0

@rallytime
Copy link
Contributor

@isbm Yeah, GitHub is having a hard time right now. It looks like they're queueing requests. I think your commits will show up when they've worked through their backlog. Thanks for updating this! We shall see how the rest of those tests go, once your commits show up. :)

@isbm
Copy link
Contributor Author

isbm commented Apr 5, 2016

@cachedout Ah, seems like finally appeared. Test made a good catch: running salt * sys.doc status.uptime was indeed returning nonsense (a freshly decorated function had yanked docstring by a decorator). Fixed.

@isbm
Copy link
Contributor Author

isbm commented Apr 6, 2016

@cachedout seems like done (one missing small lint). Other tests are kind of unrelated, or?

@cachedout cachedout merged commit 0d9a06b into saltstack:2015.8 Apr 6, 2016
@cachedout
Copy link
Contributor

Merged! Thanks for your work on this, @isbm

@isbm
Copy link
Contributor Author

isbm commented Apr 6, 2016

@cachedout Phew! 😆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants