-
-
Notifications
You must be signed in to change notification settings - Fork 746
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
RFR: Add --always-copy option to create_virtualenv method for pack virtual… #2372
RFR: Add --always-copy option to create_virtualenv method for pack virtual… #2372
Conversation
try: | ||
exit_code, _, stderr = run_command(cmd=cmd) | ||
except OSError: | ||
raise Exception('Virtualenv binary "%s" doesn\'t exist.' % virtualenv_binary) |
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.
Ideally we would also include original error message at the end.
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.
The original error message was "No such file or directory" without naming the file. This was confusing. Thus this new message.
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.
Agreed.
But OSError
doesn't necessary only mean that the file doesn't exist, it could also represent a permissions error, etc. That's why I think it's also good to include the original message somewhere.
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.
Ok, let me log the original exception and raise this new one. Does that sound good to you?
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.
I would prefer to include it in the exception message, but whatever makes you happy :P
Validated with this that sensors and actions are able to run fine with the new packages. Also, this opened up another issue https://stackstorm.atlassian.net/browse/STORM-1949 . cc: @dennybaa @Kami |
Yup |
+1 to merge. One thing |
@dennybaa Unfortunately, we cannot remove --system-site-packages from the code because old packages still need them. See https://stackstorm.slack.com/archives/stackstorm/p1452708198024063 So in st2-packages, can you set the virtualenv_opts = ['--always-copy'] in /etc/st2/st2.conf. This way new packages won't use |
* master: Add missing fixture file. Fix lint. Update changelog. Add a test case for it. Throw a more friendly exception if casting a particular parameter value fails. Change shebang in new st2ctl Include st2ctl as part of st2common Conflicts: CHANGELOG.rst
Seems like travis is stuck. I'll merge and deal with build failures if any. |
RFR: Add --always-copy option to create_virtualenv method for pack virtual…
See StackStorm/puppet-st2#148. Basically, --always-copy fails with RHEL boxes (pypa/virtualenv#565 (comment)). So I am setting a config option for existing installation to just use --system-site-packages. RHEL st2bundle won't work with --always-copy but @DoriftoShoes and I decided to deal with that later. |
If I don't get workroom to pass with StackStorm/puppet-st2#148, I'll revert this change from master for 1.3 release. cc: @Kami, @DoriftoShoes, @emedvedev, @dennybaa. |
…envs
See StackStorm/st2-packages#53 for details.
I anyways added config options for virtualenv_binary and virtualenv_opts in case we want to play around with these in the future so it's easier. The only legit change that fixed anything was
--always-copy
.This works on my dev env as well. Meanwhile, @Kami, @DoriftoShoes, @dennybaa and @armab please take a look at my comment on StackStorm/st2-packages#53 and this change.
TODO