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

pkg/ipmi: Restructure, rework and test coverage #2182

Closed
wants to merge 1 commit into from
Closed

pkg/ipmi: Restructure, rework and test coverage #2182

wants to merge 1 commit into from

Conversation

ChriMarMe
Copy link
Contributor

No description provided.

@rminnich
Copy link
Member

quick question: this is 12 commits. does it make sense to squash into fewer commits? Mainly asking, I really don't know.

rminnich
rminnich reviewed Nov 22, 2021
View reviewed changes
Copy link
Member

@rminnich rminnich left a comment

Choose a reason for hiding this comment

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

This is really nice, I only had a few nits, hope they are helpful.

pkg/ipmi/ipmi_dev.go Outdated Show resolved Hide resolved
pkg/ipmi/ipmi_dev.go Outdated Show resolved Hide resolved
pkg/ipmi/ipmi_dev_test.go Outdated Show resolved Hide resolved
pkg/ipmi/ipmi_handler.go Outdated Show resolved Hide resolved
pkg/ipmi/ipmi_linux.go Show resolved Hide resolved
pkg/ipmi/ipmi_mock.go Outdated Show resolved Hide resolved
rjoleary
rjoleary reviewed Nov 22, 2021
View reviewed changes
pkg/ipmi/constants.go Outdated Show resolved Hide resolved
pkg/ipmi/structures.go Outdated Show resolved Hide resolved
pkg/ipmi/ipmi_dev.go Outdated Show resolved Hide resolved
pkg/ipmi/ipmi_dev.go Outdated Show resolved Hide resolved
pkg/ipmi/ipmi_dev.go Outdated Show resolved Hide resolved
pkg/ipmi/ipmi_handler.go Outdated Show resolved Hide resolved
pkg/ipmi/ipmi_test.go Show resolved Hide resolved
pkg/ipmi/structures.go Outdated Show resolved Hide resolved
pkg/ipmi/structures.go Show resolved Hide resolved
pkg/ipmi/ipmi_dev.go Outdated Show resolved Hide resolved
@rminnich
Copy link
Member

overall: be sure to golint in the pkg and other directory and go vet . -- they will complain about missing comments and that sort of thing.

This is a big improvement, so let us know how we can help further to get it done.

@ChriMarMe
Copy link
Contributor Author

overall: be sure to golint in the pkg and other directory and go vet . -- they will complain about missing comments and that sort of thing.

This is a big improvement, so let us know how we can help further to get it done.

I use that per default, but it explodes with u-root. ~1k annotations over the whole project. T_T

@rjoleary rjoleary added the coverage Pull requests related to code coverage label Nov 24, 2021
@ChriMarMe ChriMarMe force-pushed the cover_ipmi branch 2 times, most recently from da0b171 to c7f4bd7 Compare November 24, 2021 12:44
@ChriMarMe ChriMarMe added the Awaiting reviewer Waiting for a reviewer. label Nov 24, 2021
@codecov
Copy link

codecov bot commented Nov 24, 2021 •
edited

Codecov Report

Merging #2182 (836ba80) into main (811f83d) will decrease coverage by 0.42%.
The diff coverage is 75.71%.

❗ Current head 836ba80 differs from pull request most recent head aa30a21. Consider uploading reports for the commit aa30a21 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2182      +/-   ##
==========================================
- Coverage   61.62%   61.19%   -0.43%     
==========================================
  Files         384      402      +18     
  Lines       39990    40663     +673     
==========================================
+ Hits        24643    24884     +241     
- Misses      14345    14733     +388     
- Partials     1002     1046      +44
Impacted Files Coverage Δ
pkg/ipmi/handler.go 0.00% <0.00%> (ø)
pkg/ipmi/linux.go 0.00% <0.00%> (ø)
pkg/ipmi/ipmi.go 60.11% <62.50%> (+60.11%) ⬆️
pkg/ipmi/mock.go 84.61% <84.61%> (ø)
pkg/ipmi/dev.go 92.50% <92.50%> (ø)
cmds/core/date/date.go 21.77% <0.00%> (-66.51%) ⬇️
pkg/cp/cp.go 39.65% <0.00%> (-29.32%) ⬇️
pkg/uroot/builder/bb.go 30.00% <0.00%> (-0.44%) ⬇️
pkg/uroot/uroot.go 50.00% <0.00%> (ø)
pkg/uroot/builder/gbb.go
... and 24 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 811f83d...aa30a21. Read the comment docs.

@ChriMarMe
Copy link
Contributor Author

Any other suggestions for improvements before I squash it all together?

@ChriMarMe
Copy link
Contributor Author

This is still not ready. Needs reimplementation of tests in the desired pattern.

@ChriMarMe ChriMarMe added Awaiting author Waiting for new changes or feedback for author. and removed Awaiting reviewer Waiting for a reviewer. labels Dec 7, 2021
@ChriMarMe ChriMarMe force-pushed the cover_ipmi branch 3 times, most recently from 4b3bfd0 to d3d7d79 Compare December 8, 2021 11:18
@ChriMarMe ChriMarMe added Awaiting reviewer Waiting for a reviewer. and removed Awaiting author Waiting for new changes or feedback for author. labels Dec 8, 2021
@hugelgupf
Copy link
Member

This is also a big change. I'd feel a lot more comfortable if we

  • had VM tests that make some IPMI call using this library
  • or if @rjoleary ran one of our internal IPMI-using tests (disk unlock) using this code patched in

@ChriMarMe
Copy link
Contributor Author

The work for the first option is way too much, especially if you have such tests in place internally already.

@rjoleary
Copy link
Contributor

rjoleary commented Jan 3, 2022

This is also a big change. I'd feel a lot more comfortable if we

* had VM tests that make _some_ IPMI call using this library

* or if @rjoleary ran one of our internal IPMI-using tests (disk unlock) using this code patched in

It possible to test SEL with QEMU's BMC sim. See http://www.linux-kvm.org/images/7/76/03x08-Juniper-Corey_Minyard-UsingIPMIinQEMU.ods.pdf

rjoleary
rjoleary reviewed Jan 3, 2022
View reviewed changes
pkg/ipmi/ipmi_test.go Show resolved Hide resolved
pkg/ipmi/mock.go Show resolved Hide resolved
pkg/ipmi/mock.go Outdated Show resolved Hide resolved
pkg/ipmi/linux.go Outdated Show resolved Hide resolved
@ChriMarMe
pkg/ipmi
aa30a21 
* Move structures to structure.go
* Move constants to constants.go
* Remove unused fdset function
* Abstract syscall implementation behind interface
* Hide syscalls behind interface to improve testability
* Add tests for ipmi
Signed-off-by: Christopher Meis <christopher.meis@9elements.com>
@ChriMarMe
Copy link
Contributor Author

This PR is tained somehow. I have to reopen i guess.

@ChriMarMe ChriMarMe closed this Jan 3, 2022
@ChriMarMe ChriMarMe mentioned this pull request Jan 3, 2022
Merged
@ChriMarMe
Copy link
Contributor Author

#2252

@ChriMarMe ChriMarMe deleted the cover_ipmi branch January 3, 2022 12:22
@ChriMarMe ChriMarMe restored the cover_ipmi branch January 31, 2022 07:35
@ChriMarMe ChriMarMe deleted the cover_ipmi branch January 31, 2022 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rjoleary rjoleary rjoleary left review comments

@rminnich rminnich Awaiting requested review from rminnich

Assignees
No one assigned
Labels
Awaiting reviewer Waiting for a reviewer. coverage Pull requests related to code coverage
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@ChriMarMe @rminnich @hugelgupf @rjoleary