facts.server, operations.server: Make Mount fact and operation portable #1382
Conversation
Using `platform.system(...)` is not a good idea because it runs on the local machine and we need to take into account the operating system of the remote machine.
DtxdF
changed the title
| @@ -208,7 +208,7 @@ class Mounts(FactBase[Dict[str, MountsDict]]): | |||
|
|
|||
| @override | |||
| def command(self): | |||
| if platform.system() == "FreeBSD": | |||
| if host.get_fact(Kernel).strip() == "FreeBSD": | |||
There was a problem hiding this comment.
Unfortunately this won’t work (can’t call facts from facts). Will need to include the check condition within the command itself.
Also noting to self should make this error properly here, as shown by the failing test case.
There was a problem hiding this comment.
Somehow the check get_fact call does work though. It returns what it should on my remote FreeBSD machine:
pyinfra inventories/physical.py fact server.Mounts
--> Fact data for: server.Mounts
{
"xx.xx.xx.xx": {
"/": {
"device": "zroot/ROOT/FreeBSD",
"type": "zfs",
...
WIthout this change it returns an error:
[x] Loaded fact server.Os
[x] cat: /proc/self/mountinfo: No such file or directory
[x] Error: could not load fact: server.Mounts
Unintentional side effect?
There was a problem hiding this comment.
Interesting! Not intentional (which is why unit tests fail since they don’t mock this path), but this feels like something that would be very useful..
Need to have a proper think about the implications, but this would be a good addition I think.
There was a problem hiding this comment.
Yes, I had doubts about that change but I have the same result as @jgelens comments, it works. The other solution is to put a conditional check through sh(1) in the command(...) function but using pyinfra for this is better.
This is what happens when I run pyinfra (excerpt):
...
--> Gathering facts...
[control-centralita] Loaded fact server.Kernel
[control-centralita] Loaded fact server.Kernel
[control-centralita] Loaded fact server.Mounts
...There was a problem hiding this comment.
Figured it out! So it does appear to work, and indeed it will in nearly all cases. It falls apart when you want to get a fact from a different host that isn't the current one. Something like:
from pyinfra import inventory
other_host_kernel = inventory.get_host("main").get_fact(Kernel)Which breaks if you use the host context within the fact. The fact module is super old and still takes state and host arguments to each function. I don't see any reason this can't be tweaked to set the state/host context before gathering facts, which would then work in all cases. Working on this today to unblock this/release.
Using
platform.system(...)is not a good idea because it runs on the local machine and we need to take into account the operating system of the remote machine.3.xat this time)scripts/dev-test.sh)scripts/dev-lint.sh)