-
-
Notifications
You must be signed in to change notification settings - Fork 457
feat: add uv facts and operations #1500
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
base: 3.x
Are you sure you want to change the base?
Conversation
maisim
left a comment
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.
Code review + functionnal tests
src/pyinfra/operations/uv.py
Outdated
| extras = " ".join(extra_args or []) if not isinstance(extra_args, str) else extra_args | ||
| install_command = f"{UV_CMD} tool install --no-progress {extras}" | ||
| uninstall_command = f"{UV_CMD} tool uninstall --no-progress {extras}" | ||
| upgrade_command = f"{install_command} --upgrade --no-progress {extras}" |
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.
--> Starting operation: Ensure cowsay tool is installed
[@Local] error: the argument '--no-progress' cannot be used multiple times
| upgrade_command = f"{install_command} --upgrade --no-progress {extras}" | |
| upgrade_command = f"{install_command} --upgrade {extras}" |
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.
But I don't quite understand why the tests are still passing without needing an update, since the command syntax shouldn't be correct anymore, right? Perhaps it's an untested case? I don't have time to look into it further right now; I'll come back to it later.
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.
Ignore my previous (now deleted) reply as I'm also confused: below is the code I see in changed files (just made the screenshot now) which doesn't have the problem (and explains why the tests passed) but doesn't explain why @maisim sees different (broken) code.
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.
Hi @morrison12
You have to look at line 175
https://github.com/pyinfra-dev/pyinfra/pull/1500/files#r2695177204
I submitted a PR on your fork to fix this:
https://github.com/morrison12/pyinfra/pull/1/changes
There is no error because no test on this part ;)
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.
oops.. indeed wrong operation.. thanks.. merged
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.
Thanx for this work !
fix: fix upgrade command for uv tool installation and add a test
Add initial set of facts and operations for
uv.Comments and Suggestions welcome.
3.xat this time)scripts/dev-test.sh)scripts/dev-lint.sh)