Skip to content

Conversation

@jordanbreen28
Copy link
Contributor

Prior to this PR, this module contained instances of Facter::Util::Resolution.exec and Facter::Util::Resolution.which, which are deprecated. This PR aims to replace these exec helpers with their supported Facter::Core::Execution counterparts.

This PR:

Replaces all Facter::Util::Resolution instances with corresponding Facter::Core::Execution exec helpers

@jordanbreen28 jordanbreen28 requested a review from a team as a code owner October 14, 2022 09:43
Prior to this PR, this module contained instances of Facter::Util::Resolution.exec and Facter::Util::Resolution.which, which are deprecated.
This PR aims to replace these exec helpers with their supported Facter::Core::Execution counterparts.

This PR:

Replaces all Facter::Util::Resolution instances with corresponding Facter::Core::Execution exec helpers
@jordanbreen28 jordanbreen28 force-pushed the cont-173/main/update_deprecated_facter branch from f44b580 to 8c9413d Compare October 14, 2022 09:44
Copy link
Collaborator

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Argh, so many unrelated issues. I wonder why this isn't using Etc.getpwuid(0).dir, at least on Linux or even POSIX.

Also, I wonder why this isn't restricted to not run on Windows.


Facter.add(:root_home) do
confine kernel: :aix
root_home = nil
Copy link
Collaborator

Choose a reason for hiding this comment

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

This line is weird and should really be inside setcode.

@chelnak
Copy link
Contributor

chelnak commented Oct 14, 2022

@ekohl Nice spot with those finds. Would you mind creating an issue so we can track them?

Copy link
Contributor

@chelnak chelnak left a comment

Choose a reason for hiding this comment

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

Changes are sound.

@ekohl raises some good points too. We can address them in another PR.

@chelnak chelnak merged commit 7280c6c into main Oct 14, 2022
@chelnak chelnak deleted the cont-173/main/update_deprecated_facter branch October 14, 2022 15:12
@ekohl
Copy link
Collaborator

ekohl commented Oct 14, 2022

I wonder why this was merged since #1278 was pretty trivial to write.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants