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.

Sign up for GitHub

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

Jump to bottom

(FACT-2656) Add solaris networking facts #1947

Merged
merged 1 commit into from
Jul 14, 2020
Merged

(FACT-2656) Add solaris networking facts #1947

merged 1 commit into from
Jul 14, 2020

Conversation

sebastian-miclea
Copy link
Contributor

No description provided.

@puppetcla
Copy link

CLA signed by all contributors.

Filipovici-Andrei
Filipovici-Andrei reviewed Jul 2, 2020
View reviewed changes
Copy link
Contributor

@Filipovici-Andrei Filipovici-Andrei left a comment

Choose a reason for hiding this comment

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

Would it be possible to add some comments explaining what is actually going on inside the methods, or split everything into more granular methods and achieve the same thing as with comments?

lib/resolvers/solaris/ffi/ffi.rb Outdated Show resolved Hide resolved
lib/resolvers/solaris/networking_resolver.rb Outdated Show resolved Hide resolved
lib/resolvers/solaris/networking_resolver.rb Outdated Show resolved Hide resolved
BogdanIrimie
BogdanIrimie reviewed Jul 7, 2020
View reviewed changes
README.md Outdated Show resolved Hide resolved
@sebastian-miclea sebastian-miclea force-pushed the FACT-2656 branch 2 times, most recently from 04905be to aa767ff Compare July 9, 2020 10:25
@sebastian-miclea sebastian-miclea changed the title Fact 2656 (FACT-2656) Add solaris networking facts Jul 9, 2020
@sebastian-miclea sebastian-miclea marked this pull request as ready for review July 9, 2020 10:27
@sebastian-miclea sebastian-miclea requested review from a team July 9, 2020 10:27
@sebastian-miclea sebastian-miclea force-pushed the FACT-2656 branch 12 times, most recently from 361501a to 9c66147 Compare July 9, 2020 14:34
@oanatmaria oanatmaria added the enhancement New feature or enhancement label Jul 10, 2020
@puppetcla
Copy link

CLA signed by all contributors.

@sebastian-miclea
Copy link
Contributor Author

jenkins please test this on debian9-64a,debian10-64a,fedora30-64a,osx1014-64a,redhat5-64a,centos8-64a,redhat7-64a,redhat7-POWERa,sles12-64a,sles15-64a,solaris10-64a,ubuntu1604-32a,ubuntu1804-64a,windows2016-64a,windows2019-64a

@sebastian-miclea
Copy link
Contributor Author

jenkins please test this on debian9-64a,debian10-64a,fedora30-64a,osx1014-64a,redhat5-64a,centos8-64a,redhat7-64a,redhat7-POWERa,sles12-64a,sles15-64a,solaris11-64a,ubuntu1604-32a,ubuntu1804-64a,windows2016-64a,windows2019-64a

@sebastian-miclea
Copy link
Contributor Author

jenkins please test this on debian9-64a,debian10-64a,fedora30-64a,osx1014-64a,redhat5-64a,centos8-64a,redhat7-64a,redhat7-POWERa,sles12-64a,sles15-64a,solaris11-64a,ubuntu1604-32a,ubuntu1804-64a,windows2016-64a,windows2019-64a

@sebastian-miclea sebastian-miclea force-pushed the FACT-2656 branch 2 times, most recently from 1baf8bf to 9018e4f Compare July 13, 2020 12:45
@sebastian-miclea
Copy link
Contributor Author

jenkins please test this on debian10-64a,fedora30-64a,osx1014-64a,redhat5-64a,centos8-64a,redhat7-64a,sles12-64a,sles15-64a,solaris11-64a,solaris114-64a,ubuntu1604-32a,ubuntu1804-64a

1 similar comment
@mihaibuzgau
Copy link
Contributor

jenkins please test this on debian10-64a,fedora30-64a,osx1014-64a,redhat5-64a,centos8-64a,redhat7-64a,sles12-64a,sles15-64a,solaris11-64a,solaris114-64a,ubuntu1604-32a,ubuntu1804-64a

Filipovici-Andrei
Filipovici-Andrei reviewed Jul 13, 2020
View reviewed changes
Copy link
Contributor

@Filipovici-Andrei Filipovici-Andrei left a comment

Choose a reason for hiding this comment

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

No unit tests for the networking resolver?

spec/facter/facts/solaris/ipaddress_interfaces_spec.rb
let(:interfaces) { nil }

it 'returns nil' do
expect(fact.call_the_resolver).to be_an_instance_of(Array).and contain_exactly
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 should check for nil not array

spec/facter/facts/solaris/macaddress_interfaces_spec.rb
let(:interfaces) { nil }

it 'returns nil' do
expect(fact.call_the_resolver).to be_an_instance_of(Array).and contain_exactly
Copy link
Contributor

Choose a reason for hiding this comment

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

This is valid for multiple other tests that check for nil

@mihaibuzgau
Copy link
Contributor

jenkins please test this on debian10-64a,fedora30-64a,osx1014-64a,redhat5-64a,centos8-64a,redhat7-64a,sles12-64a,sles15-64a,solaris11-64a,solaris114-64a,ubuntu1604-32a,ubuntu1804-64a

@sebastian-miclea
Copy link
Contributor Author

jenkins please test this on debian10-64a,fedora30-64a,osx1014-64a,redhat5-64a,centos8-64a,redhat7-64a,sles12-64a,sles15-64a,solaris11-64a,solaris114-64a,ubuntu1604-32a,ubuntu1804-64a

@sebastian-miclea sebastian-miclea force-pushed the FACT-2656 branch 2 times, most recently from a15035f to 29fbcb5 Compare July 14, 2020 13:02
@sebastian-miclea
Copy link
Contributor Author

jenkins please test this on debian10-64a,fedora30-64a,osx1014-64a,centos8-64a,redhat7-64a,sles12-64a,sles15-64a,solaris11-64a,solaris114-64a,solaris11-SPARCa,ubuntu1604-32a,ubuntu1804-64a

@mihaibuzgau mihaibuzgau merged commit d49ba48 into puppetlabs:4.x Jul 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oanatmaria oanatmaria oanatmaria left review comments

@Filipovici-Andrei Filipovici-Andrei Filipovici-Andrei left review comments

@BogdanIrimie BogdanIrimie BogdanIrimie left review comments

Assignees
No one assigned
Labels
enhancement New feature or enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@sebastian-miclea @puppetcla @mihaibuzgau @BogdanIrimie @Filipovici-Andrei @oanatmaria