host.data.VAR defaults to None

ghost commented 2 years ago

Describe the bug

When a variable referenced like so


systemd.service(
    name='Start/restart and enable all services',
    service='tink-pod.service',
    enabled=True,
    user_mode=True,
    user_name="tinkerbell",
    env = {
          'XDG_RUNTIME_DIR': '/run/user/{}'.format(host.data.tinkerbell_user_uid),
          }
    )

where the variable host.data.tinkerbell_user_uid does not exist (as in, I never declared the variable), it defaults to None. This results in an error like this:


--> Preparing operations...
    Loading: tinkerbell_persistent.py
    [tinkerbell] Failed to connect to bus: No such file or directory
    [tinkerbell] Error: could not load fact: systemd_status machine=None, user_mode=True, user_name=None
--> [tinkerbell] pyinfra error (operation=Start/restart and enable all services): No hosts remaining!

more verbose

[tinkerbell] 90f9c724a10a19c1d363761bb1e4f196347f20c2  /opt/tinkerbell/.config/systemd/user/tink-cli.service
    Loaded fact sha1_file (path=/opt/tinkerbell/.config/systemd/user/tink-cli.service)
    [tinkerbell] noop: file /opt/tinkerbell/.config/systemd/user/tink-cli.service is already uploaded
[tinkerbell] >>> sudo -H -n -u tinkerbell sh -c 'export XDG_RUNTIME_DIR=/run/user/None && ! command -v systemctl >/dev/null || systemctl --user -al list-units'
[tinkerbell] Failed to connect to bus: No such file or directory
    [tinkerbell] Error: could not load fact: systemd_status machine=None, user_mode=True, user_name=None
    Loaded fact systemd_status (machine=None, user_mode=True, user_name=None)
--> [tinkerbell] pyinfra error (operation=Start/restart and enable all services): No hosts remaining!
bash-5.1#

I think that this behavior is not desirable. It should probably throw an exception or something. Not sure if you have a reason why don't do this.

To Reproduce

deploy.py

from pyinfra import host

print(host.data.any_old_var)

Run pyinfra

bash-5.1# pyinfra @local deploy.py 
--> Loading config...
--> Loading inventory...

--> Connecting to hosts...
    [@local] Connected

--> Preparing operations...
    Loading: deploy.py
None
    [@local] Ready: deploy.py

--> Proposed changes:
    Groups: @local
    [@local]   Operations: 0   Commands: 0   

--> Beginning operation run...
--> Results:
    Groups: @local
    [@local]   Successful: 0   Errors: 0   Commands: 0/0   
bash-5.1#

Expected behavior

When a host.data.VAR does not exist I think an exception should be thrown

Meta

section of pyinfra -vvv

[tinkerbell] 90f9c724a10a19c1d363761bb1e4f196347f20c2  /opt/tinkerbell/.config/systemd/user/tink-cli.service
    Loaded fact sha1_file (path=/opt/tinkerbell/.config/systemd/user/tink-cli.service)
    [tinkerbell] noop: file /opt/tinkerbell/.config/systemd/user/tink-cli.service is already uploaded
[tinkerbell] >>> sudo -H -n -u tinkerbell sh -c 'export XDG_RUNTIME_DIR=/run/user/None && ! command -v systemctl >/dev/null || systemctl --user -al list-units'
[tinkerbell] Failed to connect to bus: No such file or directory
    [tinkerbell] Error: could not load fact: systemd_status machine=None, user_mode=True, user_name=None
    Loaded fact systemd_status (machine=None, user_mode=True, user_name=None)
--> [tinkerbell] pyinfra error (operation=Start/restart and enable all services): No hosts remaining!
bash-5.1#

cc: @jmpolom @storrgie

jmpolom commented 2 years ago

It would appear the get method in FallbackDict sets the default type to None but normally trying to access an attribute on a Python object which does not exist will result in an AttributeError. I think the behavior here deviates from what an experienced Python user would expect (an exception).

Edit: adding that I could probably fix this but it would be good to understand if the current state was chosen for a particular reason that perhaps isn't obvious.

Fizzadar commented 2 years ago

I'm pro this change in the upcoming v2 - agree that the current behaviour deviates from what you'd expect. I believe I originally made the choice to replicate jinja2 which does the same (by default), but that was many years ago :)

I think the behaviour should change to match Python:

host.data.NOT_A_VAR
>>> raises AttributeError

host.data.get('NOT_A_VAR')
>>> None

host.data.get('NOT_A_VAR', default='different-default')
>>> 'different-default'

jmpolom commented 2 years ago

I agree with the proposed behavior change. This should not deviate from standard python behavior.

Fizzadar commented 2 years ago

This is now live in v2 (and shows warnings in v1.7.!!).

pyinfra-dev / pyinfra