Skip to content
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

[change] Move python dependencies to requirements.txt file #511

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 12 additions & 6 deletions .github/dependabot.yml
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need any changes for this file.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed dependabot as it was previously in my latest commit

Original file line number Diff line number Diff line change
@@ -1,14 +1,20 @@
version: 2
updates:
- package-ecosystem: "pip" # Check for Python package updates
directory: "/" # The root directory where the Ansible role is located
- package-ecosystem: "pip"
directory: "/"
schedule:
interval: "monthly" # Check for updates weekly
interval: "weekly"
open-pull-requests-limit: 10
labels:
- "dependencies"
- "python"
commit-message:
prefix: "[deps] "
- package-ecosystem: "github-actions" # Check for GitHub Actions updates
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you removing github-actions? Please don't.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made the fix for this in my recent commit

directory: "/" # The root directory where the Ansible role is located
include: "scope"

- package-ecosystem: "github-actions"
directory: "/"
schedule:
interval: "monthly" # Check for updates weekly
interval: "weekly"
commit-message:
prefix: "[ci] "
25 changes: 25 additions & 0 deletions files/requirements.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
# Core dependencies
attrs
service_identity

# Django and related packages
django-redis~=5.4.0
django-pipeline~=3.1.0
django-cors-headers~=4.4.0
channels_redis~=4.2.0

# Database adapters
psycopg2
MySQL-python
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These packages are installed only when specific database backends are used. Since, we are installing the latest version of these packages, adding them here is not fruitful.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

modified requirements.txt file for removing Database adapters


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this extra blank line

# WSGI server
uwsgi

# Monitoring and error reporting
raven

# Email backend
django-celery-email

# Remove deprecated packages
jsonfield2==*; python_version < "0.0" # Ensures this is never installed
Comment on lines +20 to +21
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this remove the package if it was previously installed?

@nemesifier can we remove the task which ensures that this package is removed?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes we can

170 changes: 32 additions & 138 deletions tasks/pip.yml
Original file line number Diff line number Diff line change
@@ -1,39 +1,32 @@
---

- name: Update pip & related tools
pip:
name:
- pip
- setuptools
- wheel
- attrs
state: latest
virtualenv: "{{ virtualenv_path }}"
virtualenv_python: "{{ openwisp2_python }}"
retries: 5
delay: 10
register: result
until: result is success

- name: Remove jsonfield2
pip:
name:
- jsonfield2
state: absent
virtualenv: "{{ virtualenv_path }}"
virtualenv_python: "{{ openwisp2_python }}"
retries: 2
retries: 3
delay: 5
register: result
until: result is success
notify: Reload application

- name: Install openwisp2 controller and its dependencies
- name: Verify requirements.txt exists
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to verify that requirements.txt exists?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've removed this part in my latest commit

stat:
path: "{{ role_path }}/files/requirements.txt"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is this var role_path defined?

register: req_file

- name: Debug requirements file path
debug:
msg: "Looking for requirements file at {{ role_path }}/files/requirements.txt"
when: req_file.stat is defined

- name: Install Python dependencies from requirements.txt
pip:
name:
- "{{ openwisp2_controller_version }}"
- service_identity
state: latest
requirements: "{{ role_path }}/files/requirements.txt"
state: present
virtualenv: "{{ virtualenv_path }}"
virtualenv_python: "{{ openwisp2_python }}"
environment:
Expand All @@ -43,38 +36,28 @@
delay: 10
register: result
until: result is success
tags:
- molecule-idempotence-notest

- name: Install channels_redis~=4.2.0
pip:
name:
- channels_redis~=4.2.0
state: latest
virtualenv: "{{ virtualenv_path }}"
virtualenv_python: "{{ openwisp2_python }}"
retries: 1
delay: 10
register: result
until: result is success

- name: Install django-redis
- name: Install openwisp2 controller
pip:
name: "django-redis~=5.4.0"
name: "{{ openwisp2_controller_version }}"
state: present
virtualenv: "{{ virtualenv_path }}"
virtualenv_python: "{{ openwisp2_python }}"
environment:
LC_CTYPE: "en_US.UTF-8"
notify: Reload application
retries: 5
delay: 10
register: result
until: result is success
tags:
- molecule-idempotence-notest

- name: Install openwisp2 network topology and its dependencies
- name: Install openwisp2 network topology
when: openwisp2_network_topology
pip:
name: "{{ openwisp2_network_topology_version }}"
state: latest
state: present
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you change this? latest allows to upgrade the Python modules when using compatibility versioning. Same for other instances.

Copy link
Author

@shwetd19 shwetd19 Feb 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

modified pip.yml for database adapters psycopg2 & MySQL-python and to fix the state as latest

virtualenv: "{{ virtualenv_path }}"
virtualenv_python: "{{ openwisp2_python }}"
notify: Reload application
Expand All @@ -85,11 +68,11 @@
tags:
- molecule-idempotence-notest

- name: Install openwisp firmware upgrader and its dependencies
- name: Install openwisp firmware upgrader
when: openwisp2_firmware_upgrader
pip:
name: "{{ openwisp2_firmware_upgrader_version }}"
state: latest
state: present
virtualenv: "{{ virtualenv_path }}"
virtualenv_python: "{{ openwisp2_python }}"
notify: Reload application
Expand All @@ -100,11 +83,11 @@
tags:
- molecule-idempotence-notest

- name: Install openwisp monitoring and its dependencies
- name: Install openwisp monitoring
when: openwisp2_monitoring
pip:
name: "{{ openwisp2_monitoring_version }}"
state: latest
state: present
virtualenv: "{{ virtualenv_path }}"
virtualenv_python: "{{ openwisp2_python }}"
notify: Reload application
Expand All @@ -115,11 +98,11 @@
tags:
- molecule-idempotence-notest

- name: Install openwisp2_radius and its dependencies
- name: Install openwisp2_radius
when: openwisp2_radius
pip:
name: "{{ openwisp2_radius_version }}"
state: latest
state: present
virtualenv: "{{ virtualenv_path }}"
virtualenv_python: "{{ openwisp2_python }}"
notify: Reload application
Expand All @@ -130,109 +113,20 @@
tags:
- molecule-idempotence-notest

- name: Install django-cors-headers
when: openwisp2_django_cors.get('enabled')
pip:
name: "django-cors-headers~=4.4.0"
state: latest
virtualenv: "{{ virtualenv_path }}"
virtualenv_python: "{{ openwisp2_python }}"
retries: 5
delay: 10
register: result
until: result is success
notify: Reload application

- name: Install extra python packages
pip:
name: "{{ openwisp2_extra_python_packages }}"
state: latest
state: present
virtualenv: "{{ virtualenv_path }}"
virtualenv_python: "{{ openwisp2_python }}"
retries: 5
delay: 10
when: openwisp2_extra_python_packages | length > 0
retries: 3
delay: 5
register: result
until: result is success
notify: Reload application
tags: [extra_pip]

- name: Install static minification dependencies
pip:
name:
- django-pipeline~=3.1.0
state: latest
virtualenv: "{{ virtualenv_path }}"
virtualenv_python: "{{ openwisp2_python }}"
retries: 5
delay: 10
register: result
until: result is success
notify: Reload application

- name: Install uwsgi
pip:
name: uwsgi
state: latest
virtualenv: "{{ virtualenv_path }}"
virtualenv_python: "{{ openwisp2_python }}"
retries: 5
delay: 10
register: result
until: result is success
notify: Reload application

- name: Install psycopg2
when: openwisp2_database.engine in ["django.db.backends.postgresql", "django.contrib.gis.db.backends.postgis"]
pip:
name: psycopg2
state: latest
virtualenv: "{{ virtualenv_path }}"
virtualenv_python: "{{ openwisp2_python }}"
retries: 5
delay: 10
register: result
until: result is success
notify: Reload application

- name: Install MySQL-python
when: openwisp2_database.engine in ["django.db.backends.mysql", "django.contrib.gis.db.backends.mysql"]
pip:
name: MySQL-python
state: latest
virtualenv: "{{ virtualenv_path }}"
virtualenv_python: "{{ openwisp2_python }}"
retries: 5
delay: 10
register: result
until: result is success
notify: Reload application

- name: Install raven (sentry client)
when: openwisp2_sentry.get('dsn')
pip:
name: raven
state: latest
virtualenv: "{{ virtualenv_path }}"
virtualenv_python: "{{ openwisp2_python }}"
retries: 5
delay: 10
register: result
until: result is success
notify: Reload application

- name: Install django-celery-email
pip:
name: django-celery-email
state: latest
virtualenv: "{{ virtualenv_path }}"
virtualenv_python: "{{ openwisp2_python }}"
when: openwisp2_email_backend == "djcelery_email.backends.CeleryEmailBackend"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I suggested in the previous review, don't add packages to requirements.txt which are installed conditionally.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could install it anyway, it shouldn't be a big deal, what do you think?

retries: 5
delay: 10
register: result
until: result is success
notify: Reload application

- name: Install django
pip:
name: "{{ openwisp2_django_version }}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure but can we move Django also in requirements.txt file?
cc @nemesifier

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not for now, because that's configurable.

Expand Down