Skip to content
This repository has been archived by the owner on Jun 19, 2020. It is now read-only.

(FACT-2531) Fix for tests/facts/validate_file_system_size_bytes.rb #500

Merged
merged 1 commit into from
May 12, 2020

Conversation

oanatmaria
Copy link
Contributor

@oanatmaria oanatmaria commented May 11, 2020

Description of the problem:

  • for linux mountpoints all values have to be positive
  • architecture fact for 32-bit os must be i386
  • for rhel os release fact must be split 6.2 => major 6, minor 2
  • mountpoints for aix contains an additional wrong fact : "mounted"=>{:device=>"node", :filesystem=>"mounted", :options=>["options"]}
  • physical count for processors fact is 0 when "physical id" is not present in /proc/cpuinfo

Description of the fix:

  • call abs method for mountpoints bytes facts
  • force architecture value to 'i386' if it matches /i[3456]86|pentium/
  • implement os.release for rhel
  • improve mountpoints regex to exclude file's header
  • when physical count for processors fact is 0, count subdirectories from /sys/devices/system/cpu, that contains cpu(number) in name

This PR solves: tests/facts/validate_file_system_size_bytes.rb test for all oses that have mountpoints fact and os_processors_and_kernel test for all oses.

@oanatmaria oanatmaria added bugfix Something isn't working needs testing labels May 11, 2020
@oanatmaria oanatmaria requested review from a team May 11, 2020 13:04
@oanatmaria oanatmaria changed the title (FACT-231) Fix for tests/facts/validate_file_system_size_bytes.rb (FACT-2531) Fix for tests/facts/validate_file_system_size_bytes.rb May 11, 2020
@oanatmaria oanatmaria force-pushed the FACT-2531 branch 3 times, most recently from b9abfd8 to 691cbca Compare May 12, 2020 09:28
Comment on lines 57 to 59
dirs = Dir.entries('/sys/devices/system/cpu').reject { |dir| dir =~ /\.+/ }

dirs.count { |dir| dir =~ /cpu[0-9]+$/ }
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can use count directly, as you are passing a block and counting specific entries:

Dir.entries('/sys/devices/system/cpu').count { |dir| dir =~ /cpu[0-9]+$/ }

Also maybe the method name can be more suggestive: physical_devices_count or something like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -10,6 +10,7 @@ class Architecture
def call_the_resolver
fact_value = Facter::Resolvers::Uname.resolve(:machine)
fact_value = 'amd64' if fact_value == 'x86_64'
fact_value = fact_value =~ /i[3456]86|pentium/ ? 'i386' : fact_value
Copy link
Contributor

Choose a reason for hiding this comment

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

fact_value = i386 if `fact_value =~ /i[3456]86|pentium/

You already have a good value in fact_value change it only if you need to

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -10,6 +10,8 @@ class Architecture
def call_the_resolver
fact_value = Facter::Resolvers::Uname.resolve(:machine)

fact_value = fact_value =~ /i[3456]86|pentium/ ? 'i386' : fact_value
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as on lib/facts/debian/architecture.rb

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -19,7 +19,7 @@ def read_mount(fact_name)
@fact_list[:mountpoints] = {}
output = Facter::Core::Execution.execute('mount', logger: log)
output.split("\n").map do |line|
next if line =~ /\snode\s|---|procfs|ahafs/
next if line =~ /node|---|procfs|ahafs/
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this change covered in tests? i don't see aix/mountpoints_spec.rb to have changed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the change is in fixtures/mount

@gimmyxd
Copy link
Contributor

gimmyxd commented May 12, 2020

Would be useful to describe in the commit message what this fixes, as the PR title states is fixing an acceptance tests but is not clear what was the issue.

@Filipovici-Andrei Filipovici-Andrei merged commit b0a7135 into master May 12, 2020
@Filipovici-Andrei Filipovici-Andrei deleted the FACT-2531 branch May 12, 2020 11:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bugfix Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants