New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow extra user plugins in pbs_benchpress #853

Merged
merged 1 commit into from Oct 27, 2018

Conversation

Projects
None yet
4 participants
@hirenvadalia
Contributor

hirenvadalia commented Oct 24, 2018

Bug/feature Description

  • Currently pbs_benchpress in PTL has hard coded list of plugins which it is loading from ptl.utils.plugins directory and so it doesn't allow user to do extra stuff before and after test runs. For example force restart server before each test case or send test execution status to some central database etc...

Affected Platform(s)

  • All platform which PTL supports

Cause / Analysis / Design

Solution Description

  • Added new option in pbs_benchpress which takes list of custom plugins which will get loaded along with hard coded plugins in pbs_benchpress

Testing logs/output

Checklist:

***For further information please visit the Developer Guide Home.***

@hirenvadalia

This comment has been minimized.

Contributor

hirenvadalia commented Oct 24, 2018

Since this is change in pbs_benchpress command itself so no selftest is required. Also no new test require since there no core product change.
But please let me know if anyone thinks otherwise.

_msg += ' name of plugin'
logging.error(_msg)
sys.exit(1)
mod, clsname = plugin.split('=')

This comment has been minimized.

@bhroam

bhroam Oct 24, 2018

Contributor

Consider doing split('=', 1) since you are only accepting two arguments. If someone did foo=bar=baz, this will barf.

This comment has been minimized.

@hirenvadalia

hirenvadalia Oct 25, 2018

Contributor

I like using split('=', 1) but here we are safe by using just split('=') because '=' is not allowed in module name or class name in python.

xxx@xxx:~/code/myplugins$ vi 't=.py'
xxx@xxx:~/code/myplugins$ python
Python 2.7.15rc1 (default, Apr 15 2018, 21:51:34) 
[GCC 7.3.0] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import t=
  File "<stdin>", line 1
    import t=
            ^
SyntaxError: invalid syntax
>>> import 't='
  File "<stdin>", line 1
    import 't='
              ^
SyntaxError: invalid syntax
>>> exit()
xxx@xxx:~/code/myplugins$ vi myplugin.py
xxx@xxx:~/code/myplugins$ python
Python 2.7.15rc1 (default, Apr 15 2018, 21:51:34) 
[GCC 7.3.0] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import myplugin
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "myplugin.py", line 9
    class My=Info(Plugin):
            ^
SyntaxError: invalid syntax
>>>

But if you still think we should use split('=', 1) then I am happy to make change.

This comment has been minimized.

@bhroam

bhroam Oct 25, 2018

Contributor

I guess it comes down to how you want to barf. If the split() returns 3 or more values, you'll barf with a ValueError trying to set 2 values where there are more. If you use split('=', 1), you'll barf when you try and import the module with a SyntaxError. Depending how much you care, you could do the split('=', 1) and then check for an '=' in clsname. It depends how you want to exit.

This comment has been minimized.

@hirenvadalia

hirenvadalia Oct 26, 2018

Contributor

@bhroam I have changed code to split('=', 1) instead split('=').

@@ -264,6 +271,8 @@ if __name__ == '__main__':
testfiles = CliUtils.expand_abs_path(val)
elif o == '-t':
testsuites = val
elif o == '--extra-plugins':
extra_plugins = val

This comment has been minimized.

@bhroam

bhroam Oct 24, 2018

Contributor

Not something to do now, but we might consider using argparse instead of getopt. It does a really good job with argument parsing.

This comment has been minimized.

@hirenvadalia

hirenvadalia Oct 25, 2018

Contributor

Absolutely agree. Also along with argparse we should really consider moving away from Nose as it is kind a dead project (last release was ~3.5 year back) and there is very good alternative called pytest (very actively maintained and much powerful/flexible than Nose)

@kjakkali

Looks good.

@bhroam

bhroam approved these changes Oct 26, 2018

Looks good. I sign off.

@hirenvadalia

This comment has been minimized.

Contributor

hirenvadalia commented Oct 27, 2018

Thanks @kjakkali and @bhroam for review. @PBSPro/pbspro-maintainers PR is ready for merge.

@mike0042 mike0042 merged commit 07b3d14 into PBSPro:master Oct 27, 2018

4 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
@hirenvadalia

This comment has been minimized.

Contributor

hirenvadalia commented Oct 29, 2018

Thanks @mike0042.

@hirenvadalia hirenvadalia deleted the hirenvadalia:dyn-plugin branch Oct 29, 2018

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