[change] Added openwisp2_pip_extra_args: "--upgrade" to force pip to upgrade deps#575
Conversation
Piyushrathoree
changed the title
Piyushrathoree
changed the title
|
@pandafy PTAL |
pandafy
left a comment
pandafy
left a comment
There was a problem hiding this comment.
Good start @Piyushrathoree!
I am afraid that --upgrade might not always work when we use tarball URL as source. E.g. when we set openwisp-controller version in the ansible playbook
openwisp2_controller_version: openwisp-controller @ https://github.com/openwisp/openwisp-controller/tarball/masterIn such scenarios, we would need to use state: forcereinstall.
Using forcereinstall by default will slow down the playbook, since it will make pip reinstall packages even when the latest package have been installed.
I can think of exposing pip.extra_args argument through ansible variable openwisp2_pip_extra_args. The default value for this argument will be openwisp2_pip_extra_args: "--upgrade". When needed, the user can override the variable in the playbook to use `openwisp2_pip_extra_args: "--upgrade --force-reinstall" which will force-reinstall the packages.
We will need to add this new variable in https://github.com/openwisp/ansible-openwisp2/blob/master/docs/user/role-variables.rst, and explain the usage with a comment. You can take reference from other settings mentioned in the file.
value --upgrade and can be overridden with --upgrade --force-reinstall
This reverts commit 98367c9.
015e499 to
73ed71d
Compare
thanks @pandafy |
Co-authored-by: Gagan Deep <the.one.above.all.titan@gmail.com>
| # Extra arguments passed to pip when reinstalling Python packages with --force-reinstall | ||
| # By default, packages are upgraded on each playbook run. | ||
| # Can be overridden, for example: | ||
| # openwisp2_pip_extra_args: "--upgrade --force-reinstall" |
There was a problem hiding this comment.
| # Extra arguments passed to pip when reinstalling Python packages with --force-reinstall | |
| # By default, packages are upgraded on each playbook run. | |
| # Can be overridden, for example: | |
| # openwisp2_pip_extra_args: "--upgrade --force-reinstall" | |
| # Extra arguments passed to pip when installing Python packages. | |
| # By default, packages are upgraded on each playbook run. | |
| # Can be overridden, for example: | |
| # openwisp2_pip_extra_args: "--upgrade --force-reinstall" |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (2)**/*.{yml,yaml,j2}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{yml,yaml,j2,sh}📄 CodeRabbit inference engine (AGENTS.md)
Files:
🔇 Additional comments (2)
📝 WalkthroughWalkthroughAdds a new public Ansible variable openwisp2_pip_extra_args (default "--upgrade") in role defaults, documents it in role variables, and forwards it as extra_args to multiple ansible.builtin.pip tasks so pip installs for controller, requirements files, optional components, DB drivers, and celery-email can receive additional pip CLI arguments. Sequence Diagram(s)sequenceDiagram
participant Playbook
participant Defaults
participant PipModule
participant RequirementsFiles
Playbook->>Defaults: read openwisp2_pip_extra_args
Playbook->>PipModule: call ansible.builtin.pip with extra_args={{ openwisp2_pip_extra_args }}
PipModule->>RequirementsFiles: install/update packages from requirements files
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
♻️ Duplicate comments (1)
docs/user/role-variables.rst (1)
34-38:⚠️ Potential issue | 🟡 MinorClarify the
openwisp2_pip_extra_argsdescription to match actual default behavior.Line 34 implies
--force-reinstallis part of the normal behavior, but Line 38 sets default to--upgrade. Please describe--force-reinstallas an optional override example only.✏️ Proposed doc wording
- # Extra arguments passed to pip when reinstalling Python packages with --force-reinstall + # Extra arguments passed to pip when installing Python packages. # By default, packages are upgraded on each playbook run. # Can be overridden, for example: # openwisp2_pip_extra_args: "--upgrade --force-reinstall"As per coding guidelines, "Changes: If the change affects behavior that is already documented, the documentation must be updated to reflect the new behavior".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/user/role-variables.rst` around lines 34 - 38, The documentation incorrectly implies --force-reinstall is part of default behavior for openwisp2_pip_extra_args; update the description to state that the default value of openwisp2_pip_extra_args is "--upgrade" (packages are upgraded on each playbook run) and mention --force-reinstall only as an optional override example (e.g., openwisp2_pip_extra_args: "--upgrade --force-reinstall") so the docs reflect actual default behavior rather than suggesting force-reinstall is applied by default.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@docs/user/role-variables.rst`:
- Around line 34-38: The documentation incorrectly implies --force-reinstall is
part of default behavior for openwisp2_pip_extra_args; update the description to
state that the default value of openwisp2_pip_extra_args is "--upgrade"
(packages are upgraded on each playbook run) and mention --force-reinstall only
as an optional override example (e.g., openwisp2_pip_extra_args: "--upgrade
--force-reinstall") so the docs reflect actual default behavior rather than
suggesting force-reinstall is applied by default.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d421369a-1cb6-4428-aafd-6c7db36bfc06
📒 Files selected for processing (3)
defaults/main.ymldocs/user/role-variables.rsttasks/pip.yml
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build debian12
- GitHub Check: Build ubuntu2204
- GitHub Check: Build ubuntu2404
- GitHub Check: Build debian13
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{md,rst,txt}
📄 CodeRabbit inference engine (Custom checks)
**/*.{md,rst,txt}: Changes: If the change affects behavior that is already documented, the documentation must be updated to reflect the new behavior
Features: Documentation must mention the new feature; for heavily UI-related features, a new section or page is appropriate
Files:
docs/user/role-variables.rst
🔇 Additional comments (2)
defaults/main.yml (1)
237-238: Good default and variable exposure.Introducing
openwisp2_pip_extra_argswith default--upgradeis a clean, backward-compatible way to make reruns upgrade-aware while preserving override flexibility.tasks/pip.yml (1)
46-46: Nice reuse of configurable pip args across requirements installs.The additions consistently parameterize pip
extra_argsin the affected requirement-based tasks, which improves maintainability and supports override scenarios from inventory/group vars.Also applies to: 126-126, 158-158, 177-177, 217-217
|
Hi @Piyushrathoree 👋, This pull request has been marked as stale due to 139 days of inactivity after changes were requested. As a result, any linked issues are being unassigned from you so other contributors can pick them up. However, you can still continue working on this PR! If you push new commits or respond to the review feedback:
If you need more time or have questions about the requested changes, please let us know. We're happy to help! 🤝 |
|
Hi @Piyushrathoree 👋, This PR has been inactive for 140 days since changes were requested. Are you still working on it? If yes, push new commits or reply to let us know. If you've moved on, please close the PR or comment so another contributor can pick it up. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tasks/pip.yml (1)
17-113:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftMajor package tasks should also accept
extra_argsto support forcereinstall.The PR discussion noted that tarball URL packages need
state: forcereinstallor--force-reinstallin extra_args because--upgradealone doesn't detect changes when the tarball URL is static. Currently, only requirements-file tasks receiveopenwisp2_pip_extra_args, so even if a user overrides the variable to"--upgrade --force-reinstall", these major packages (controller, network-topology, firmware-upgrader, monitoring, radius) won't be force-reinstalled.For static tarball URLs like
https://.../tarball/master,state: latestcannot detect content changes since the URL remains constant. This leaves a gap: users cannot force-reinstall tarball-based packages through the new configuration variable.🔧 Proposed fix: Add extra_args to tarball package tasks
Apply to each of the five major package installation tasks (controller, network-topology, firmware-upgrader, monitoring, radius):
- name: Install openwisp2 controller and its dependencies ansible.builtin.pip: name: - "{{ openwisp2_controller_version }}" - service_identity state: latest virtualenv: "{{ virtualenv_path }}" virtualenv_python: "{{ openwisp2_python }}" + extra_args: "{{ openwisp2_pip_extra_args }}" environment: LC_CTYPE: "en_US.UTF-8" notify: Reload applicationRepeat this pattern for the network-topology (line 57), firmware-upgrader (line 72), monitoring (line 87), and radius (line 102) tasks.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tasks/pip.yml` around lines 17 - 113, The five pip install tasks for the major packages (the tasks titled "Install openwisp2 controller and its dependencies", "Install openwisp2 network topology and its dependencies", "Install openwisp firmware upgrader and its dependencies", "Install openwisp monitoring and its dependencies", and "Install openwisp2_radius and its dependencies") must accept the extra_args variable so tarball installs can be force-reinstalled; add extra_args: "{{ openwisp2_pip_extra_args }}" to each of those ansible.builtin.pip task blocks (keeping their existing name, when conditions like openwisp2_network_topology/openwisp2_firmware_upgrader/openwisp2_monitoring/openwisp2_radius, virtualenv and virtualenv_python unchanged) so users can pass "--force-reinstall" or other pip flags via openwisp2_pip_extra_args.defaults/main.yml (1)
238-238: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winEnhance the comment to match documentation clarity.
The inline comment is too brief and doesn't explain the variable's purpose or usage. The documentation in
docs/user/role-variables.rstprovides a much clearer explanation that should be reflected here.📝 Suggested comment improvement
-# variable for default deps upgrade +# Extra arguments passed to pip when installing Python packages from requirements files. +# By default, packages are upgraded on each playbook run. +# Can be overridden, for example: openwisp2_pip_extra_args: "--upgrade --force-reinstall"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@defaults/main.yml` at line 238, Replace the terse inline comment "# variable for default deps upgrade" with a descriptive comment that matches the wording in docs/user/role-variables.rst: state the variable's name, purpose, accepted values (e.g., true/false or list), default behavior, and example usage; update the comment directly above the variable definition so readers can understand what the "default deps upgrade" setting does and how to use it without opening the docs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@defaults/main.yml`:
- Line 238: Replace the terse inline comment "# variable for default deps
upgrade" with a descriptive comment that matches the wording in
docs/user/role-variables.rst: state the variable's name, purpose, accepted
values (e.g., true/false or list), default behavior, and example usage; update
the comment directly above the variable definition so readers can understand
what the "default deps upgrade" setting does and how to use it without opening
the docs.
In `@tasks/pip.yml`:
- Around line 17-113: The five pip install tasks for the major packages (the
tasks titled "Install openwisp2 controller and its dependencies", "Install
openwisp2 network topology and its dependencies", "Install openwisp firmware
upgrader and its dependencies", "Install openwisp monitoring and its
dependencies", and "Install openwisp2_radius and its dependencies") must accept
the extra_args variable so tarball installs can be force-reinstalled; add
extra_args: "{{ openwisp2_pip_extra_args }}" to each of those
ansible.builtin.pip task blocks (keeping their existing name, when conditions
like
openwisp2_network_topology/openwisp2_firmware_upgrader/openwisp2_monitoring/openwisp2_radius,
virtualenv and virtualenv_python unchanged) so users can pass
"--force-reinstall" or other pip flags via openwisp2_pip_extra_args.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4159f5ee-33d0-4887-8bbf-b9cdd5314696
📒 Files selected for processing (2)
defaults/main.ymltasks/pip.yml
📜 Review details
🔇 Additional comments (6)
defaults/main.yml (1)
239-239: LGTM!tasks/pip.yml (5)
41-53: LGTM!
122-133: LGTM!
155-166: LGTM!
175-186: LGTM!
216-229: LGTM!
nemesifier
changed the title
github-project-automation
Bot
moved this to In progress
in OpenWISP Priorities for next releases
There was a problem hiding this comment.
♻️ Duplicate comments (1)
docs/user/role-variables.rst (1)
34-34:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDocumentation incorrectly describes the variable's behavior.
Line 34 states "when reinstalling Python packages with --force-reinstall", but the default value is
"--upgrade", which upgrades packages, not force-reinstalls them. This misrepresents the default behavior.The past review comment suggested: "Extra arguments passed to pip when installing Python packages" (not "reinstalling with --force-reinstall").
📝 Proposed fix to align documentation with actual behavior
- # Extra arguments passed to pip when reinstalling Python packages with --force-reinstall + # Extra arguments passed to pip when installing Python packages. # By default, packages are upgraded on each playbook run.As per coding guidelines, the past review comment suggested this exact wording to accurately describe the variable's purpose.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/user/role-variables.rst` at line 34, Update the documentation line currently reading "# Extra arguments passed to pip when reinstalling Python packages with --force-reinstall" to accurately reflect the variable's behavior: describe it as extra arguments passed to pip when installing Python packages and mention the default value "--upgrade" (e.g., "Extra arguments passed to pip when installing Python packages (default: \"--upgrade\")") so the text matches the actual default and intent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@docs/user/role-variables.rst`:
- Line 34: Update the documentation line currently reading "# Extra arguments
passed to pip when reinstalling Python packages with --force-reinstall" to
accurately reflect the variable's behavior: describe it as extra arguments
passed to pip when installing Python packages and mention the default value
"--upgrade" (e.g., "Extra arguments passed to pip when installing Python
packages (default: \"--upgrade\")") so the text matches the actual default and
intent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6522c501-2bcf-4e45-b89f-4af2592791b9
📒 Files selected for processing (2)
defaults/main.ymldocs/user/role-variables.rst
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{yml,yaml,j2}
📄 CodeRabbit inference engine (AGENTS.md)
Avoid unnecessary blank lines inside Jinja, YAML, and shell blocks.
Files:
defaults/main.yml
**/*.{yml,yaml,j2,sh}
📄 CodeRabbit inference engine (AGENTS.md)
Write comments only when they explain why code is shaped a certain way. Put comments before the relevant block instead of scattering them inside it.
Files:
defaults/main.yml
**/*.{md,rst,txt}
📄 CodeRabbit inference engine (Custom checks)
**/*.{md,rst,txt}: Changes: If the change affects behavior that is already documented, the documentation must be updated to reflect the new behavior
Features: Documentation must mention the new feature; for heavily UI-related features, a new section or page is appropriate
Files:
docs/user/role-variables.rst
🔇 Additional comments (1)
defaults/main.yml (1)
239-240: LGTM!
github-project-automation
Bot
moved this from In progress
to Done
in OpenWISP Priorities for next releases
3 participants
Checklist
Reference to Existing Issue
Closes #572
Description of Changes
update the pip.yml file with extra-args:"--upgrade" which make sure that every dependency is updated.