-
Notifications
You must be signed in to change notification settings - Fork 100
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
Clairvoyance v2.0.0 - Package, Async, Linting #40
Conversation
@nikitastupin |
|
||
. /opt/pysetup/.venv/bin/activate | ||
|
||
exec python3 -m clairvoyance "$@" |
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 wonder why do we need exec
here 🤔
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.
It's a docker practice allowing to forward everything (signals, env, ...) to the new shell.
You can read more about this:
https://stackoverflow.com/questions/39082768/what-does-set-e-and-exec-do-for-docker-entrypoint-scripts
Hey @c3b5aw, Thank you very much for the PR. I've skimmed the changes and left few comments. Other than that we're good. Regarding further feature requests, I suggest to merge this one as soon as possible (after finishing the ToDo of course) and then add things iteratively (it's much easier and faster to review small PRs). Would you be willing to proceed this way? I can go through existing Issues and Pull Requests, de-duplicate and close stale ones so that we'll know what to focus on next. |
Thank you! I'm going to review and merge closer to the end of this week. |
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'll make some minor changes after the update but overall looks good!
Seems to work fine with exception of publishing to PyPI - the "clairvoyance" name is already claimed. I've filled the details at #42 and probably will start solving it next week. |
Changes
Repo structure
Docker
Workflows
cd.yml
-> Require
${{ secrets.PYPI_TOKEN }}
to be set.test.yml
Code
ToDo