tasks: replace os_identification with facts #459
Conversation
| @@ -5,7 +5,7 @@ | |||
| include BoltSpec::Plans | |||
|
|
|||
| it 'file needs downloaded and needs uploaded' do | |||
| expect_task('peadm::os_identification') | |||
| expect_task('facts') | |||
There was a problem hiding this comment.
I'm sure that we need to mock the facts task, but I'm not sure how at the moment 🤔 . And I'm not sure how the tests passed previously. Wasn't peadm::os_identification mocked any where?
| @@ -5,7 +5,7 @@ | |||
| include BoltSpec::Plans | |||
|
|
|||
| it 'file needs downloaded and needs uploaded' do | |||
| expect_task('peadm::os_identification') | |||
| allow_task('facts').be_called_times(1).with_targets('local://localhost').always_return({ 'os' => {'family' => 'RedHat'} }) | |||
There was a problem hiding this comment.
This should pass now. Please keep in mind that it currently only tests for Linux. There were no tests for Windows.
|
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. |
|
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. |
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. |
the facts task is vendored into bolt. It supports gathering facts from systems with and without facter installed.
the facts module itself has tests for it: 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.
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. |
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?