You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by GitBox <gi...@apache.org> on 2020/08/11 08:25:18 UTC
[GitHub] [skywalking-python] Humbertzhang opened a new pull request #63: [Plugin] check version of packages when install plugins
Humbertzhang opened a new pull request #63:
URL: https://github.com/apache/skywalking-python/pull/63
<!-- Uncomment the following checklist WHEN AND ONLY WHEN you're adding a new plugin -->
<!--
- [ ] Add a test case for the new plugin
- [ ] Add a component id in [the main repo](https://github.com/apache/skywalking/blob/master/oap-server/server-bootstrap/src/main/resources/component-libraries.yml#L415)
- [ ] Add a logo in [the UI repo](https://github.com/apache/skywalking-rocketbot-ui/tree/master/src/views/components/topology/assets)
- [ ] Add the library/module that this plugin instruments into the `setup.py` (`extras_require/test`), such as `pymysql`, `flask`, `django`, etc.
- [ ] Rebuild the `requirements.txt` by running `tools/env/build_requirements_(linux|windows).sh`
-->
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-python] kezhenxu94 commented on a change in pull request #63: [Plugin] check version of packages when install plugins
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #63:
URL: https://github.com/apache/skywalking-python/pull/63#discussion_r468664611
##########
File path: skywalking/plugins/__init__.py
##########
@@ -38,7 +41,65 @@ def install():
continue
logger.debug('installing plugin %s', modname)
plugin = importer.find_module(modname).load_module(modname)
+
+ logger.debug('checking version for plugin %s', modname)
+ supported = pkg_version_check(plugin, modname)
+ if not supported:
+ continue
+
if not hasattr(plugin, 'install') or inspect.ismethod(getattr(plugin, 'install')):
logger.warning('no `install` method in plugin %s, thus the plugin won\'t be installed', modname)
continue
plugin.install()
+
+
+_operators = {
+ '<': lambda cv, ev: cv < ev,
+ '<=': lambda cv, ev: cv < ev or cv == ev,
+ '=': lambda cv, ev: cv == ev,
+ '>=': lambda cv, ev: cv > ev or cv == ev,
+ '>': lambda cv, ev: cv > ev,
+ '!=': lambda cv, ev: cv != ev
+}
+
+
+def pkg_version_check(plugin, modname):
+ supported = True
+
+ # no version rules was set, no checks
+ if not hasattr(plugin, "version_rule"):
+ return supported
+
+ pkg_name = plugin.version_rule.get("name")
+ rules = plugin.version_rule.get("rules")
+
+ try:
+ current_pkg_version = pkg_resources.get_distribution(pkg_name).version
+ except pkg_resources.DistributionNotFound:
+ supported = False
+ logger.warning("plugin %s didn\'t find the corresponding package %s, thus won't be installed",
+ modname, pkg_name)
+ return supported
+
+ # check all rules
+ for rule in rules:
+ idx = 2 if rule[1] == '=' else 1
+ symbol = rule[0:idx]
+ expect_pkg_version = rule[idx:]
+
+ current_version = version.parse(current_pkg_version)
+ expect_version = version.parse(expect_pkg_version)
+ f = _operators.get(symbol) or None
+
+ # version rule parse error, take it as no more version rules and return True.
+ if not f:
+ logger.warning("plugin %s version rule %s error. only allow >,>=,=,<=,<,!= symbols", modname, rule)
+ return supported
Review comment:
We can simply raise an exception and exit immediately, this kind of exception should be fixed at development phase and never happen at runtime
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-python] kezhenxu94 commented on a change in pull request #63: [Plugin] check version of packages when install plugins
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #63:
URL: https://github.com/apache/skywalking-python/pull/63#discussion_r468663509
##########
File path: skywalking/plugins/__init__.py
##########
@@ -38,7 +41,65 @@ def install():
continue
logger.debug('installing plugin %s', modname)
plugin = importer.find_module(modname).load_module(modname)
+
+ logger.debug('checking version for plugin %s', modname)
+ supported = pkg_version_check(plugin, modname)
+ if not supported:
+ continue
+
if not hasattr(plugin, 'install') or inspect.ismethod(getattr(plugin, 'install')):
logger.warning('no `install` method in plugin %s, thus the plugin won\'t be installed', modname)
continue
plugin.install()
+
+
+_operators = {
+ '<': lambda cv, ev: cv < ev,
+ '<=': lambda cv, ev: cv < ev or cv == ev,
+ '=': lambda cv, ev: cv == ev,
+ '>=': lambda cv, ev: cv > ev or cv == ev,
+ '>': lambda cv, ev: cv > ev,
+ '!=': lambda cv, ev: cv != ev
+}
+
+
+def pkg_version_check(plugin, modname):
+ supported = True
+
+ # no version rules was set, no checks
+ if not hasattr(plugin, "version_rule"):
+ return supported
+
+ pkg_name = plugin.version_rule.get("name")
+ rules = plugin.version_rule.get("rules")
+
+ try:
+ current_pkg_version = pkg_resources.get_distribution(pkg_name).version
+ except pkg_resources.DistributionNotFound:
+ supported = False
+ logger.warning("plugin %s didn\'t find the corresponding package %s, thus won't be installed",
+ modname, pkg_name)
Review comment:
When failed to get the version, we can just consider it as supported as the built-in libraries should always throw exception here
##########
File path: skywalking/plugins/__init__.py
##########
@@ -38,7 +41,65 @@ def install():
continue
logger.debug('installing plugin %s', modname)
plugin = importer.find_module(modname).load_module(modname)
+
+ logger.debug('checking version for plugin %s', modname)
+ supported = pkg_version_check(plugin, modname)
+ if not supported:
+ continue
+
if not hasattr(plugin, 'install') or inspect.ismethod(getattr(plugin, 'install')):
logger.warning('no `install` method in plugin %s, thus the plugin won\'t be installed', modname)
continue
plugin.install()
+
+
+_operators = {
+ '<': lambda cv, ev: cv < ev,
+ '<=': lambda cv, ev: cv < ev or cv == ev,
+ '=': lambda cv, ev: cv == ev,
+ '>=': lambda cv, ev: cv > ev or cv == ev,
+ '>': lambda cv, ev: cv > ev,
+ '!=': lambda cv, ev: cv != ev
+}
+
+
+def pkg_version_check(plugin, modname):
+ supported = True
+
+ # no version rules was set, no checks
+ if not hasattr(plugin, "version_rule"):
+ return supported
+
+ pkg_name = plugin.version_rule.get("name")
+ rules = plugin.version_rule.get("rules")
+
+ try:
+ current_pkg_version = pkg_resources.get_distribution(pkg_name).version
+ except pkg_resources.DistributionNotFound:
+ supported = False
+ logger.warning("plugin %s didn\'t find the corresponding package %s, thus won't be installed",
+ modname, pkg_name)
+ return supported
+
+ # check all rules
+ for rule in rules:
+ idx = 2 if rule[1] == '=' else 1
+ symbol = rule[0:idx]
+ expect_pkg_version = rule[idx:]
+
+ current_version = version.parse(current_pkg_version)
+ expect_version = version.parse(expect_pkg_version)
+ f = _operators.get(symbol) or None
+
+ # version rule parse error, take it as no more version rules and return True.
+ if not f:
+ logger.warning("plugin %s version rule %s error. only allow >,>=,=,<=,<,!= symbols", modname, rule)
+ return supported
Review comment:
We can simply raise an exception and exit immediately, this kind of exception should be fixed at development phase and never happened at runtime
##########
File path: skywalking/plugins/__init__.py
##########
@@ -38,7 +41,65 @@ def install():
continue
logger.debug('installing plugin %s', modname)
plugin = importer.find_module(modname).load_module(modname)
+
+ logger.debug('checking version for plugin %s', modname)
+ supported = pkg_version_check(plugin, modname)
+ if not supported:
+ continue
Review comment:
[1] move `logger.debug('checking version for plugin %s', modname)` into the `if not supported` block, and reword the message to tell the users that the current version is not supported by the plugin
##########
File path: skywalking/plugins/__init__.py
##########
@@ -38,7 +41,65 @@ def install():
continue
logger.debug('installing plugin %s', modname)
plugin = importer.find_module(modname).load_module(modname)
+
+ logger.debug('checking version for plugin %s', modname)
+ supported = pkg_version_check(plugin, modname)
+ if not supported:
+ continue
+
if not hasattr(plugin, 'install') or inspect.ismethod(getattr(plugin, 'install')):
logger.warning('no `install` method in plugin %s, thus the plugin won\'t be installed', modname)
continue
plugin.install()
+
+
+_operators = {
+ '<': lambda cv, ev: cv < ev,
+ '<=': lambda cv, ev: cv < ev or cv == ev,
+ '=': lambda cv, ev: cv == ev,
+ '>=': lambda cv, ev: cv > ev or cv == ev,
+ '>': lambda cv, ev: cv > ev,
+ '!=': lambda cv, ev: cv != ev
+}
+
+
+def pkg_version_check(plugin, modname):
+ supported = True
+
+ # no version rules was set, no checks
+ if not hasattr(plugin, "version_rule"):
+ return supported
+
+ pkg_name = plugin.version_rule.get("name")
+ rules = plugin.version_rule.get("rules")
+
+ try:
+ current_pkg_version = pkg_resources.get_distribution(pkg_name).version
+ except pkg_resources.DistributionNotFound:
+ supported = False
+ logger.warning("plugin %s didn\'t find the corresponding package %s, thus won't be installed",
+ modname, pkg_name)
+ return supported
+
+ # check all rules
+ for rule in rules:
+ idx = 2 if rule[1] == '=' else 1
+ symbol = rule[0:idx]
+ expect_pkg_version = rule[idx:]
+
+ current_version = version.parse(current_pkg_version)
+ expect_version = version.parse(expect_pkg_version)
+ f = _operators.get(symbol) or None
+
+ # version rule parse error, take it as no more version rules and return True.
+ if not f:
+ logger.warning("plugin %s version rule %s error. only allow >,>=,=,<=,<,!= symbols", modname, rule)
+ return supported
+
+ if not f(current_version, expect_version):
+ supported = False
+ logger.warning("plugin %s need package %s version follow rules %s ,current version " +
+ "is %s, thus won\'t be installed", modname, pkg_name, str(rules), current_pkg_version)
Review comment:
Move this kind of message to where the invoker calls this method, I mark [1] in the previous comment
<img width="914" alt="image" src="https://user-images.githubusercontent.com/15965696/89915973-97fe0b80-dc29-11ea-80cf-5179c7482a02.png">
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-python] kezhenxu94 commented on a change in pull request #63: [Plugin] check version of packages when install plugins
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #63:
URL: https://github.com/apache/skywalking-python/pull/63#discussion_r468527399
##########
File path: skywalking/plugins/__init__.py
##########
@@ -38,7 +41,64 @@ def install():
continue
logger.debug('installing plugin %s', modname)
plugin = importer.find_module(modname).load_module(modname)
+
+ ok = pkg_version_check(plugin, modname)
+ if not ok:
+ continue
+
if not hasattr(plugin, 'install') or inspect.ismethod(getattr(plugin, 'install')):
logger.warning('no `install` method in plugin %s, thus the plugin won\'t be installed', modname)
continue
plugin.install()
+
+
+_operators = {
+ '<': lambda cv, ev: cv < ev,
+ '<=': lambda cv, ev: cv < ev or cv == ev,
+ '=': lambda cv, ev: cv == ev,
+ '>=': lambda cv, ev: cv > ev or cv == ev,
+ '>': lambda cv, ev: cv > ev,
+ '!=': lambda cv, ev: cv != ev
+}
+
+
+def pkg_version_check(plugin, modname):
Review comment:
Can you add some unit tests for this function? Just to check the `operators` work correctly
##########
File path: skywalking/plugins/__init__.py
##########
@@ -38,7 +41,64 @@ def install():
continue
logger.debug('installing plugin %s', modname)
plugin = importer.find_module(modname).load_module(modname)
+
+ ok = pkg_version_check(plugin, modname)
+ if not ok:
Review comment:
Log some debug message here
##########
File path: skywalking/plugins/__init__.py
##########
@@ -38,7 +41,64 @@ def install():
continue
logger.debug('installing plugin %s', modname)
plugin = importer.find_module(modname).load_module(modname)
+
+ ok = pkg_version_check(plugin, modname)
Review comment:
Let's rename `ok` to `supported` for better readability
##########
File path: skywalking/plugins/__init__.py
##########
@@ -38,7 +41,64 @@ def install():
continue
logger.debug('installing plugin %s', modname)
plugin = importer.find_module(modname).load_module(modname)
+
+ ok = pkg_version_check(plugin, modname)
+ if not ok:
+ continue
+
if not hasattr(plugin, 'install') or inspect.ismethod(getattr(plugin, 'install')):
logger.warning('no `install` method in plugin %s, thus the plugin won\'t be installed', modname)
continue
plugin.install()
+
+
+_operators = {
+ '<': lambda cv, ev: cv < ev,
+ '<=': lambda cv, ev: cv < ev or cv == ev,
+ '=': lambda cv, ev: cv == ev,
+ '>=': lambda cv, ev: cv > ev or cv == ev,
+ '>': lambda cv, ev: cv > ev,
+ '!=': lambda cv, ev: cv != ev
+}
+
+
+def pkg_version_check(plugin, modname):
+ ok = True
+
+ if hasattr(plugin, "version_rule"):
Review comment:
reverse the condition to reduce nested blocks
```python
if not hasattr(plugin, "version_rule"):
return ok
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-python] kezhenxu94 merged pull request #63: [Plugin] check version of packages when install plugins
Posted by GitBox <gi...@apache.org>.
kezhenxu94 merged pull request #63:
URL: https://github.com/apache/skywalking-python/pull/63
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-python] Humbertzhang commented on a change in pull request #63: [Plugin] check version of packages when install plugins
Posted by GitBox <gi...@apache.org>.
Humbertzhang commented on a change in pull request #63:
URL: https://github.com/apache/skywalking-python/pull/63#discussion_r468532218
##########
File path: skywalking/plugins/__init__.py
##########
@@ -38,7 +41,64 @@ def install():
continue
logger.debug('installing plugin %s', modname)
plugin = importer.find_module(modname).load_module(modname)
+
+ ok = pkg_version_check(plugin, modname)
+ if not ok:
Review comment:
Is it ok to log `logger.debug('checking version for plugin %s', modname)` ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org