-
Notifications
You must be signed in to change notification settings - Fork 11
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
Use virtualenv instead of user pip #44
Conversation
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.
Thank you! I added a few comments. Additionally, I may be blind, but where are you setting pretalx_python
?
@@ -6,7 +6,7 @@ After=network.target | |||
[Service] | |||
User={{ pretalx_system_user_prefix }}%i | |||
Group={{ pretalx_system_user_prefix }}%i | |||
WorkingDirectory=/home/{{ pretalx_system_user_prefix }}%i/.local/lib/python{{ pretalx_system_python_version }}/site-packages/pretalx | |||
ExecStart=/home/{{ pretalx_system_user_prefix }}%i/.local/bin/gunicorn --bind unix:/run/gunicorn/pretalx_%i --workers {{ pretalx_service_workers }} --max-requests {{ pretalx_service_workers_max_requests }} --max-requests-jitter {{ pretalx_service_workers_max_requests_jitter }} pretalx.wsgi | |||
WorkingDirectory={{ pretalx_virtualenv }}/lib/python{{ pretalx_system_python_version }}/site-packages/pretalx |
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.
You've removed the parametrisation (%i
) in this service file, fundamentally changing how it's used. I don't think that's a good idea – while I personally don't run this ansible role anymore (in favour of something already using venvs, albeit more customized ones), changing the behaviour like that doesn't seem great.
@@ -16,6 +16,7 @@ | |||
- gcc | |||
- gettext | |||
- git | |||
- npm |
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.
Included this in a separate commit, just to get it merged while we talk about the rest here.
state: latest # noqa package-latest | ||
virtualenv: "{{ pretalx_virtualenv }}" |
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.
You're using |default(omit)
in some places but not in others, is that intentional?
become: true | ||
become_user: "{{ pretalx_system_user }}" | ||
|
||
- name: Compile pretalx javascript |
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.
Hmm, I don't think this is quite correct. Instead, we can just replace the collectstatic
and compress
commands with rebuild
– rebuild runs collectstatic, compress and also compilemessages.
I should probably have been clearer: I wanted to get some feedback about the idea of using virtualenvs in the first place, which is why I opened this PR as a draft in the first place. Will rework it a bit (and split the npm part) and open this again. |
Yeah, absolutely in favour of using venvs in general, thank you for looking into this! Didn't mean to reject the PR entirely, just the implementation details :) |
I think it is cleaner to use virtualenv instead of a pip --user:
I'm opening as a draft PR because this will break arch compatibility.