Open romanchyla opened 1 year ago
romanchyla
commented
1 year ago I tried testing it against previous releases (2.0--2.5) and got the same results. Maybe the behaviour was always there -- however, I only discovered it while debugging a deployment, which previously worked. And I was very surprised to see that host.get_fact runs with sudo
Fizzadar
commented
1 year ago Hi @romanchyla this is actually the correct behaviour, _sudo is passed between nested operation and deploy function calls (so a caller can expect all nested operations to share the same context). There was a bugfix related in 2.5.3 which may have accidentally hidden this.
For full context here's a slightly expanded example:
from pyinfra import host
from pyinfra.api import deploy
from pyinfra.facts.server import Home
@deploy("")
def do():
# This should be SSH user's home unless do is called with _sudo, in which case root's home
print("DO", host.get_fact(Home))
# This should always be SSH user's home, whether do is called with sudo or not
print("DO", host.get_fact(Home, _sudo=False))
# Will show extension with root home (because sudo)
extensions(_sudo=True)
# Will show extension with same home as caller to do
extensions()
# Will always use SSH user as _sudo here overrides any passed into do
extensions(_sudo=False)
@deploy("")
def extensions():
print("EXTENSIONS", host.get_fact(Home))
do()
do(_sudo=True)If I run this locally I get:
DO /Users/nick
DO /Users/nick
EXTENSIONS /var/root
EXTENSIONS /Users/nick
EXTENSIONS /Users/nick
DO /var/root
DO /Users/nick
EXTENSIONS /var/root
EXTENSIONS /var/root
EXTENSIONS /Users/nickThis is the intended behaviour as above, with the possibility of explicitly disabling sudo available as well.
romanchyla
commented
1 year ago Thank you @Fizzadar for the detailed example, it helps a lot.
Though what do you think about this?
@deploy("")
def do(_sudo=True):
click.secho(host.get_fact(Home), fg="yellow")
extensions()
@deploy("")
def extensions():
click.secho(host.get_fact(Home), fg="yellow")
def doit():
do()
do(_sudo=False)
do(_sudo=True)doit()
will print:
/home/ANT.AMAZON.COM/rchyla
/home/ANT.AMAZON.COM/rchyla
/home/ANT.AMAZON.COM/rchyla
/home/ANT.AMAZON.COM/rchyla
[@local] sudo password:
/root
/rootshouldn't the first call (with implicit _sudo=True) also produce /root?
Seems bit inconsistent still.
The changed behaviour, based on a different context in which it gets executed, is rather unobvious - while reading the code. It may make up for some interesting debugging :)
Fizzadar
commented
1 year ago @romanchyla agreed, I think this case should behave at you specify; quick explanation below on why that doesn't currently and a possible solution.
@deploy("")
def do(_sudo=True):
click.secho(host.get_fact(Home), fg="yellow")
extensions()In this case the _sudo is ignored because the global arguments are collected by the @deploy decorator which happens before any call to do. It should be possible to support this though by inspecting the default arguments of do in the decorator and applying those accordingly.
The deploy decorator is found here and the global arguments are selected from the caller of do here. The above could work by providing defaults to the kwargs passed into pop_global_arguments based on the default arguments in the decorated deploy function.
Describe the bug
A changed behaviour in a new release - context (some shared state) seems to be leaking or getting modified between calls.
To Reproduce
This test fails (see below info about version info).
It passes if I call
get_factbefore calling the nested deployment; it also passes if I set the global _sudoExpected behavior
host.get_fact(Home) should return the home of the user who is executing the deployment (which used to be the default behaviour). It should not return different results if (some) prior call is made -- it should not share environment/context with the parent. If it does, it should do it consistently.
Meta