Skip to content
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

Event Script plugin #38

Merged
merged 3 commits into from
Feb 15, 2017
Merged

Conversation

tokejepsen
Copy link
Contributor

This is an event plugin that executes a script specified in the job parameters.

This is an event plugin that executes a script specified in the job parameters.
@jamesthinkbox
Copy link
Contributor

Thanks for this contribution! A couple small things...

  1. Please fix the CI checks that failed.
  2. Please add a README.md that describes the script and how it is used,

@tokejepsen
Copy link
Contributor Author

Why did it fail on the print statement? Is the CI on python 3?

@jamesthinkbox
Copy link
Contributor

Our CI checks against both Python 2.7 and 3. In Python 3, the content of a print statement must be enclosed in parentheses. Parentheses are backwards compatible to 2.x, so should work for both.

@tokejepsen
Copy link
Contributor Author

That should do it :)

@jamesthinkbox
Copy link
Contributor

Thanks! I'm going to merge this now.

I do have a couple of recommendations for improvements: After retrieving the script's path from the ExtraInfo, run a path mapping on it to convert it to the Slave's current platform (so that the script works cross-platform). After doing the path mapping, confirm that the script file is reachable from the Slave before calling execfile().

@jamesthinkbox jamesthinkbox merged commit fe565e4 into ThinkboxSoftware:master Feb 15, 2017
@tokejepsen
Copy link
Contributor Author

I do have a couple of recommendations for improvements: After retrieving the script's path from the ExtraInfo, run a path mapping on it to convert it to the Slave's current platform (so that the script works cross-platform). After doing the path mapping, confirm that the script file is reachable from the Slave before calling execfile().

All good suggestions :) Unfortunately I can't test that currently, so maybe it should be in an issue?

@jamesthinkbox
Copy link
Contributor

Understood. I've created #39 to keep track of it. Thanks again for this nice addition.

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.

2 participants