Halfway through the review, many small things and a few bigger things to fix. Overall not terrible. But take the time to step back, understand clearly the code review and revisit the entire p-r with the comments in mind and improve it.
Don't insult other projects, kubespray is cool and suits a purpose.
I'd like to see here a proper rust struct for the file module config built as proper rust code and simply serialized to the ansible module format instead of a bunch of json! invocations feeling very fragile. We should research if there already exist ansible rust crates. I doubt we will find mature and solid ones but it's worth looking we could be surprised.
The file spec is probably the correct one, all that is lacking is a function to serialize it directly to ansible file module json.
I have some doubts about this trait.
Also many things this does require sudo (like creating a user) which might end up in cloud init for a reason or another, I think it could be misleading to have a single trait with implementations all over the place from cloud init to calling an ansible module to install a package, etc.
This is ugly. Use a long string with format! or askama templates like we do in other places.
Are there alternatives to systemd unit files to make sure it restarts on reboot? What about running the agent itself as a podman container? It would probably have to be privileged but that would reduce the configuration burden on the host and centralize our logic around podman instead of spreading it to systemd.
This could very well use a harmony id from harmony_types. I like the id format, it is unique, relatively short and contains a timestamp.
Why do we need that? It is yet another dependency? Can't we pass the cloud init any other way when creating the vm? Do we need that for cloud init as I'm assuming or something else?
I tend to disagree with the comment that we need to be tool specific here.
This is tricky and is actually an architectural problem in harmony. Ensure ready is executed eagerly on a topology, but this topology won't necessarily be running a kvm related workload this run and/or might be doing the kvm setup in an earlier task before calling the kvm dependent scores. I am pretty sure we have a ROADMAP entry on that topic of topology initialization dependency. Add a reference here to the roadmap entry with a TODO so we capture this use case when we get to this work.
This feels straight up wrong. KvmVmScore (sounds generic) depending on CloudInit (specific) is a crime against good architecture and naming.
Use args vec, more readable. Also true for all other Command::new calls.
feels like ansible should have a purpose built module here too. Don't be lazy and call shell commands. Calling shell commands through ansible is completely useless as they're not truly idempotent in the way that file and package installations are.
ef418f2f96
Merge pull request 'feat: Disable ipv4 address conflict detection score. This is useful when setting up bonds as the wrong mac may get a dhcp offer and then the system will perceive it as a conflict when it sets up the bond correctly' (#263) from feat/disableDadScore into master
6267c2757f
feat: Disable ipv4 address conflict detection score. This is useful when setting up bonds as the wrong mac may get a dhcp offer and then the system will perceive it as a conflict when it sets up the bond correctly