search

puppetlabs / puppetlabs-peadm

A Puppet module defining Bolt plans used to automate Puppet Enterprise deployments
Apache License 2.0
29 stars 53 forks source link

tasks: replace os_identification with facts #459

Open bastelfreak opened 1 month ago

bastelfreak commented 1 month ago

the facts task is vendored into bolt. It supports gathering facts from systems with and without facter installed.

Summary

Provide a detailed description of all the changes present in this pull request.

Additional Context

Add any additional context about the problem here.

Related Issues (if any)

Mention any related issues or pull requests.

Checklist

Changes include test coverage?

Have you updated the documentation?

CoMfUcIoS commented 1 month ago

I believe that this change breaks the workflow where the jump host doesn't have puppet installed and/or managed by puppet. Hence, I don't see it as improvement, but as removing a functionality, but I might also be wrong. Were there any problems with the current implementation that we need to fix? I find the current implementation robust enough (and simple enough for being maintained any further) to change it to something "untested" on Windows OSes now.

bastelfreak commented 1 month ago

I mentioned this already in the PR where the os_identification task was added. The facts task does not rely on Puppet. It can use if it's present.

There's already:

I don't see a reason why PEADM needs to reinvent the wheel for another implementation.

CoMfUcIoS commented 1 month ago

I don't see a reason why PEADM needs to reinvent the wheel for another implementation.

My concerns about this are that the current implementation is tested thoroughly on various Windows platforms, and it works, while the one proposed in this PR (which I am not saying is wrong or faulty) is completely untested on Windows. Why change something that is tested and working at this point of time with something that is completely untested? I am not confident enough to change it at this point of time. If there were tests attached to this PR for windows platform, then obviously we wouldn't have to argue about it.

bastelfreak commented 2 weeks ago

If there were tests attached to this PR for windows platform, then obviously we wouldn't have to argue about it.

the facts module itself has tests for it: https://github.com/puppetlabs/puppetlabs-facts/blob/main/spec/acceptance/windows_spec.rb

I don't see any windows acceptance tests in peadm, so I cannot enhance those. If you explicitly want unit tests for the task, I can add them.

Why change something that is tested and working at this point of time with something that is completely untested?

Because Perforce teams heavily suffer from not-invented-here syndrome. And "completely untested" is not true.

There's a perfectly working, and already tested in itself, alternative solution. And that's not just any open source project, it's from the same company. And having multiple implementations is really confusing for your users and it just wastes development resources. I think it makes way more sense to have a single implementation. And if that lacks features, enhance it instead of inventing another solution.

Also all the existing acceptance tests pass and use the task already.