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

Add task to overwrite package dependencies #169

Merged
merged 2 commits into from
Feb 29, 2024
Merged

Add task to overwrite package dependencies #169

merged 2 commits into from
Feb 29, 2024

Conversation

bbe-dw
Copy link
Contributor

@bbe-dw bbe-dw commented Feb 9, 2024

Added a task to modify the version of a pip package already present in the netbox requirements. For example, with netbox v3.4.10, there are issues due to the rq package requiring downgrading to version 1.13.0 or installing django-rq v2.10.1 instead of the 2.7.0 set in the requirement.

@lae
Copy link
Owner

lae commented Feb 9, 2024

I think this is effectively a reimplementation of netbox_pip_constraints (#36)? I believe that existing role variable should also work for this use case, unless you're running into some issue with it?

@bbe-dw
Copy link
Contributor Author

bbe-dw commented Feb 9, 2024

The netbox_pip_constraints variable can be used to add new packages that are not already present in the netbox requirements file.

This doesn't work, generating a conflict for a package that is already in the netbox requirements file.

In the case of my example, wanting to install django-rq 2.10.1 via the netbox_pip_constraints variable doesn't work because django-rq 2.7.0 is present in the requirements file, causing a conflict.

@lae
Copy link
Owner

lae commented Feb 9, 2024

The netbox_pip_constraints variable can be used to add new packages that are not already present in the netbox requirements file.

the default for the constraints role variable already does include django-rq, which works fine and is how we got around django-rq not working before, so that can't be completely true.

Are you saying overriding the default with django-rq==2.10.1 causes the constraint to fail (and do you have an error for reference)?

@lae
Copy link
Owner

lae commented Feb 9, 2024

tmp]$ pip install -r req.txt -c con.txt
ERROR: Cannot install django-rq==2.7.0 because these package versions have conflicting dependencies.

The conflict is caused by:
    The user requested django-rq==2.7.0
    The user requested (constraint) django-rq==2.10.1

To fix this you could try to:
1. loosen the range of package versions you've specified
2. remove package versions to allow pip attempt to solve the dependency conflict

ERROR: ResolutionImpossible: for help visit https://pip.pypa.io/en/latest/topics/dependency-resolution/#dealing-with-dependency-conflicts

hm okay, I guess constraints doesn't completely override what's in requirements.

@lae
Copy link
Owner

lae commented Feb 9, 2024

I need to think about this a bit more. Having two role variables that overlap in responsibility is not ideal imo.

@bbe-dw
Copy link
Contributor Author

bbe-dw commented Feb 9, 2024

We could imagine a task that takes the "netbox_pip_constraints" variable and then deletes the requirements lines whose package names match.
This would make it possible to use the variable netbox_pip_constraints.
It could be an idea, but maybe there's something better.

@lae
Copy link
Owner

lae commented Feb 9, 2024

Also I think this breaks updating on git installations because modifying files in the source tree prevents git from cleanly pulling updates or switching refs.

I'm thinking that maybe one option would be to copy the requirements file, check if there are any pin requests in the constraints role variable (i.e. any constraints that specifically use ==), and remove the pins in the copied requirements file but keep them in the constraints file...

@lae
Copy link
Owner

lae commented Feb 9, 2024

then deletes the requirements lines whose package names match.

according to python docs this will be a problem. the constraints file only specifies versions, not what packages to install, so if they're not in the requirements file (either explicitly or implicitly as a dependency) then they won't get installed.

But yeah, that's similar to the idea I had, just modify requirements based off the constraints role variable.

Alternatively the proposed role variable could replace the constraints role variable and we just do away with setting constraints altogether to modify the requirements file (well, a copy) in the first place. That would put the burden of fixing dependencies on the admin, though (constraints were added to this role in order to let us manage what versions are bad as part of the role, too, in a sense. on that note, I'm actually kind of unsure what breakage you're facing with django-rq on 3.4.10 that requires upgrading it).

@lae
Copy link
Owner

lae commented Feb 23, 2024

@bbe-dw coming back to this for a bit, but does

netbox_pip_packages:
  - django-rq==2.10.1

work for your use case to override the requirement (installs it right after requirements.txt is taken care of)? Or does that break idempotency....

@bbe-dw
Copy link
Contributor Author

bbe-dw commented Feb 23, 2024

Iae

I'm looking this afternoon for this solution (delete the version in the requirements.txt file if the package is present in netbox_pip_packages in order to avoid adding the "netbox_pip_pin_requirements" variable.

I'll let you know soon

@bbe-dw
Copy link
Contributor Author

bbe-dw commented Feb 23, 2024

I modified it so that packages present in the requirement remain in the file, but the version is removed if the package is present in the "netbox_pip_constraints" variable.
This doesn't break the idempotency

I can't use the "netbox_pip_packages" variable because the installation is done in a task after the one that installs those present in the constraints and requirements. If I leave the package in the requirements file, it will automatically install the latest version and the next task to install the pacakges in the netbox_pip_packages variable will fail.

Copy link
Owner

@lae lae left a comment

Choose a reason for hiding this comment

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

Okay I guess there's no way around modifying requirements.txt1, then. However, we can't modify the original source file directly because this role also handles git repositories, as stated earlier.

My suggestion is to maintain a copy of the requirements.txt file in the netbox_shared_path, modify that, and use that file in the pip task. And if we're doing that, might as well put the constraints file there, too.

so to summarize,
ansible.builtin.template to drop constraints file in netbox_shared_path
ansible.builtin.copy from netbox_current_path to netbox_shared_path,
ansible.builtin.replace the file in netbox_shared_path for constraints that pin a specific version (when: "'==' in item"?)2
ansible.builtin.pip to use the files in netbox_shared_path instead

Could you update the tasks to function like that? (I would also recommend updating the module names of any tasks that are changed to be fully qualified.)

Footnotes

  1. I referred to this last time but leaving here for future reference: https://pip.pypa.io/en/stable/topics/dependency-resolution/#possible-solutions

  2. Ideally I'd want to do some more sanity checking here but anything more would get too complex

tasks/deploy_netbox.yml Outdated Show resolved Hide resolved
tasks/deploy_netbox.yml Show resolved Hide resolved
tasks/deploy_netbox.yml Outdated Show resolved Hide resolved
@bbe-dw
Copy link
Contributor Author

bbe-dw commented Feb 29, 2024

I've modified and added, as you pointed out, filtering on == (or >=, =~, != ...)
I had to add "changed_when: false" because otherwise it made the playbook non idempoten

Copy link
Owner

@lae lae left a comment

Choose a reason for hiding this comment

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

looks good to me.

I'm going to update the when conditional to only replace if constraints include == because I don't want to commit to anything more right now just in case it breaks netbox requirements for some people. Like imagine requirements has ==5.0 and constraints had >4.2. These are compatible but blindly removing ==5.0 might cause unintentional breakage especially, for example, if latest is a 6.0 with removed import paths.

More sanity checking for compatibility would be desired if we wanted to override versions with any of the other version specifiers (which I mentioned in earlier footnotes would get complex. I'm not even sure where to begin with handling that).

@lae lae merged commit f22efc6 into lae:develop Feb 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants