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 some pipx operations & facts #1039

Draft
wants to merge 3 commits into
base: 2.x
Choose a base branch
from

Conversation

maisim
Copy link
Contributor

@maisim maisim commented Nov 21, 2023

Start pipx support,

Some tests do not pass but I am not sure of the way to write them in the correct way

WIP

@maisim maisim force-pushed the pipx_operations_and_facts branch 2 times, most recently from 08c1edc to 892eae2 Compare November 22, 2023 09:07
Copy link
Member

@Fizzadar Fizzadar left a comment

Choose a reason for hiding this comment

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

Hi @maisim! Apologies for the epic delay getting a proper review on this. Left a few comments, let me know what you think!

def requires_command(self, pipx=None):
return pipx or self.pipx_command

def command(self, pipx=None):
Copy link
Member

Choose a reason for hiding this comment

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

I think we can just always use pipx as the command, there's no case where we'd have multiple versions alongside each other I think?

# We should always use the --force flag because
# if install it's called, that meen the version missmatch
# so we need the --force flag to get the good version installed
install_command = [pipx, "install", "--force"]
Copy link
Member

Choose a reason for hiding this comment

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

I think force should be a keyword argument for the operation, rather than added by default. We should be hitting the upgrade command if a package is already installed?

if packages:
current_packages = host.get_fact(PipxPackages, pipx=pipx)

if current_packages is None:
Copy link
Member

Choose a reason for hiding this comment

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

This should never be the case, facts return a default value so can drop this check.

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.

None yet

2 participants