pyinfra-dev / pyinfra

pyinfra turns Python code into shell commands and runs them on your servers. Execute ad-hoc commands and write declarative operations. Target SSH servers, local machine and Docker containers. Fast and scales from one server to thousands.
https://pyinfra.com
MIT License
3.91k stars 382 forks source link

pyinfra cli facts does not respect global arguments #758

Closed bauen1 closed 2 years ago

bauen1 commented 2 years ago

Describe the bug

e.g. pyinfra -vvv --sudo --dry --limit unused -- inventory.py fact systemd.SystemdStatus does not respect use_sudo_password as defined in the inventory.

To Reproduce

Create an inventory with a host and use_sudo_password set, invoke pyinfra -vvv --sudo --dry --limit unused -- inventory.py fact systemd.SystemdStatus on it and observe that it (wrongly) asks for a sudo password.

Expected behavior

Global arguments should be applied, this allows setting the sudo password, etc., in the inventory.

Meta

Version used was https://github.com/Fizzadar/pyinfra/commit/c8e2f8569798d72b0668d8ac9a5d1fdace74d2ff

Things from discord

@fizzadar And I also have a question about https://github.com/Fizzadar/pyinfra/commit/2cd2e1a5b3ad6389a9ae815b28f2326fe55a4f42

Right now I'm trying to run a fact against a host, that has use_sudo_password defined in the inventory, this should cause pyinfra to go straight to uploading the password without first trying to run the command without providing a password¹, but it seemingly doesn't do it, it even prompts me for the password, despite it being in the inventory as a string

¹It's an issue because that sends mail to the (other) system admins, who rightfully get a bit annoyed at me

Oh, and it's not set in the config, just in the inventory data, perhaps that is tripping things up

And I think it might be because in https://github.com/Fizzadar/pyinfra/blob/current/pyinfra/api/facts.py#L187-L188 which is invoked directly when using the CLI get get a fact, nothing is done to "apply global arguments", especially because current_op_global_kwargs doesn't seem to be set by the cli before calling get_facts

Fizzadar commented 2 years ago

V2 should properly fix this (#759 implemented). The facts API has been rewritten and simplified such that it properly handles all the execution related global arguments. Hoping to have an alpha release out this week!

Fizzadar commented 2 years ago

Support for this is now released in v2 :)