Skip to content

(CONT-173) - Updating deprecated facter instances #1277

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

Merged
merged 1 commit into from
Oct 14, 2022

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.

@@ -39,7 +39,7 @@ def returnt_root_home
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.

4 participants