Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tasks: replace os_identification with facts #459

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

bastelfreak
Copy link
Collaborator

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

  • 🟢 Spec tests.
  • 🟢 Acceptance tests.

Changes include test coverage?

  • Yes
  • Not needed

Have you updated the documentation?

  • Yes, I've updated the appropriate docs
  • Not needed

@@ -5,7 +5,7 @@
include BoltSpec::Plans

it 'file needs downloaded and needs uploaded' do
expect_task('peadm::os_identification')
expect_task('facts')
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'} })
Copy link
Collaborator Author

@bastelfreak bastelfreak Jul 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should pass now. Please keep in mind that it currently only tests for Linux. There were no tests for Windows.

@CoMfUcIoS
Copy link
Contributor

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
Copy link
Collaborator Author

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
Copy link
Contributor

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.
@bastelfreak
Copy link
Collaborator Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants