-
Notifications
You must be signed in to change notification settings - Fork 5
Allow running hooks handler using command and/or relative path #55
Conversation
69479e0
to
830a11c
Compare
830a11c
to
6b4de15
Compare
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.
Looks good, I'd make one change that I suggested below (changing handlerCommand to be just executablePath to prevent any potential problems with virtualenv and awkward Python setups).
scripts/smoke-test.js
Outdated
const relativePathBase = path.join(testDir, 'package.json'); | ||
const pythonPath = path.relative(relativePathBase, which.sync('python')); | ||
const executablePath = path.relative(relativePathBase, which.sync('dredd-hooks-python')); | ||
const handlerCommand = `${pythonPath} ${executablePath}`; |
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.
Can't this just be dredd-hooks-python
(executablePath)? I would have though that dredd-hooks-python being an executable should be executable 😀.
If so, it also means that if python
globally is a different version than the one in the shebang line for dredd-hooks-python
I could get broken installation. Perhaps an example use-case of this behaviour would be installing the hooks as a virtualenv and then running outside the virtual env (this can be how Homebrew/system packages worked if hooks could be distributed like that).
Repro:
$ python3 -m venv venv
$ source venv/bin/activate.fish
(venv) $ pip install dredd-hooks
(venv) $ which dredd-hooks-python
/Users/kyle/Projects/apiaryio/api-elements.js/venv/bin/dredd-hooks-python
(venv) $ head -n1 (which dredd-hooks-python)
#!/Users/kyle/Projects/apiaryio/api-elements.js/venv/bin/python3
(venv) $ which python
/Users/kyle/Projects/apiaryio/api-elements.js/venv/bin/python
(venv) $ python --version
Python 3.7.3
(venv) $ deactivate
$ python --version
Python 2.7.16
$ which python
/usr/local/bin/python
Executing venv/bin/dredd-hooks-python
in venv as executable:
$ ./venv/bin/dredd-hooks-python
Starting Dredd Python hooks handler
Executing venv/bin/dredd-hooks-python
in venv using global python (means transitive deps from venv not available too):
$ python ./venv/bin/dredd-hooks-python
Traceback (most recent call last):
File "./venv/bin/dredd-hooks-python", line 6, in <module>
from pkg_resources import load_entry_point
File "/usr/local/lib/python2.7/site-packages/pkg_resources/__init__.py", line 3191, in <module>
@_call_aside
File "/usr/local/lib/python2.7/site-packages/pkg_resources/__init__.py", line 3175, in _call_aside
f(*args, **kwargs)
File "/usr/local/lib/python2.7/site-packages/pkg_resources/__init__.py", line 3204, in _initialize_master_working_set
working_set = WorkingSet._build_master()
File "/usr/local/lib/python2.7/site-packages/pkg_resources/__init__.py", line 583, in _build_master
ws.require(__requires__)
File "/usr/local/lib/python2.7/site-packages/pkg_resources/__init__.py", line 900, in require
needed = self.resolve(parse_requirements(requirements))
File "/usr/local/lib/python2.7/site-packages/pkg_resources/__init__.py", line 786, in resolve
raise DistributionNotFound(req, requirers)
pkg_resources.DistributionNotFound: The 'dredd-hooks==0.2.0' distribution was not found and is required by the application
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.
Yes, dredd-hooks-python
would work.
// find out what is the relative path to the Python hooks executable so
// we can use it in the test suite (with Python hooks this is not necessary,
// plain 'dredd-hooks-python' would work fine, but we want to test here that
// relative paths and complex commands work with the test suite)
But this is a smoke test in which I need to test that relative paths and commands like bundle exec something
will work as well. For that reason I'm artificially making this more complex. I'm wondering how to fix the comment so my intent is clearer.
You have a good point about the Python path though, I should probably take it from the shebang. It works fine locally within virtualenv or on CI, but who knows what setups someone might have.
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 makes sense if you are testing that particular flow, didn't try to run (or break) this particular test locally.
e5ddda7
to
3267ce7
Compare
🎉 This PR is included in version 1.0.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Addressing #4 and unblocking apiaryio/dredd-hooks-ruby#37