-
-
Notifications
You must be signed in to change notification settings - Fork 31
Add sudo_options & 'env' parameters to run_command
#167
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
Conversation
vsoch
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! Please see my suggestion to use environ instead of env across functions. Then the final tweaks that will make it good for merge:
- please update the CHANGELOG with detail to the addition
- bump the version in spython/version.py
vsoch
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me! Let me know when you are all finished and we can merge and release.
|
Looks like linting is failing - I would check your black version against the CIs https://app.circleci.com/pipelines/github/singularityhub/singularity-cli/301/workflows/c5bb4bec-2faf-430f-9282-664ce0462716/jobs/1162/parallel-runs/0/steps/0-114 |
|
Looks like a lot of files(47) need to reformatted to pass that test and there are differences between different checks, what should I do? |
|
heyo! Sorry for the delay! I just updated master with pinning a black version (and making the lints) so you should be able to rebase and update the PR here. fb33495 |
merge: running black-20.8b1 for linting, pinning version in testing (singularityhub#168)
|
~ Done ~ |
|
Thank you @cceyda ! https://pypi.org/project/spython/0.0.85/ |
add the
sudo_optionsparameter to some commands that were missing it (instance: start & stop, list_instances)add an
envparameter for passing environment variables toClient.instanceandClient.executeI have been using spython to start some services dynamically and often found myself wanting to pass some environment variables, adding an 'env' parameter like this was a cleaner solution for me.
If this is problematic and there is another way to pass many environment variables cleanly let me know