FS-1123: Supermicro (SMC) BIOS config Get, Set and Reset support via SUM #392
Conversation
splaspood
force-pushed
the
supermicro-bios-FS-1123
branch
2 times, most recently
from
8cc9139 to
318630b
Compare
|
@splaspood is this ready for review? set reviewers in that case. |
splaspood
force-pushed
the
supermicro-bios-FS-1123
branch
from
1638530 to
678cddf
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #392 +/- ##
==========================================
+ Coverage 37.48% 42.44% +4.95%
==========================================
Files 78 61 -17
Lines 6405 5544 -861
==========================================
- Hits 2401 2353 -48
+ Misses 3782 2970 -812
+ Partials 222 221 -1 ☔ View full report in Codecov by Sentry. |
| @@ -1,4 +1,4 @@ | |||
| FROM mcr.microsoft.com/devcontainers/go:1-1.21-bullseye | |||
| FROM mcr.microsoft.com/devcontainers/go:1-1.22-bullseye | |||
There was a problem hiding this comment.
why are we using a Microsoft golang container?
There was a problem hiding this comment.
I'd guess simply because that's what's suggested in the devcontainer documentation/examples. My understanding is there's nothing really special about the devcontainer image(s) so we could replace that with anything.
| SetStdout([]byte) | ||
| SetStderr([]byte) | ||
| SetExitCode(int) |
There was a problem hiding this comment.
Do these have to be exported?
There was a problem hiding this comment.
I'd have to review the code in more detail, but my guess would be these are exported for use in testing. They are exported here because they were exported in the ironlib code that this was directly copied from.
There was a problem hiding this comment.
when we're using a mock library, these would not be required
internal/executor/fake_executor.go
Outdated
There was a problem hiding this comment.
Why not use mockery or gomock here?
There was a problem hiding this comment.
Only because this already existed in our code base. I don't have anything against using these libs...
| "github.com/pkg/errors" | ||
| ) | ||
|
|
||
| // Executor interface lets us implement dummy executors for tests |
There was a problem hiding this comment.
this comment says this is for tests but this interface is used the the sum package which is not a test package. which one is it, for tests or not?
Also, why is this a package at all? As far as i can tell this is only used in the sum package. This should probably just be move there.
There was a problem hiding this comment.
It's external to the sum package with the expectation that there could/would be other similar utilities needing this in the future.
There was a problem hiding this comment.
what the the other utilities that will need and or implement this? if we dont have other uses we should hold off creating this.
This also is a big interface and its purpose and value add are not very clear. are we planning on replacing all calls to the os/exec with this?
"Do not define interfaces before they are used: without a realistic example of usage, it is too difficult to see whether an interface is even necessary, let alone what methods it ought to contain." - https://go.dev/wiki/CodeReviewComments#interfaces
There was a problem hiding this comment.
I do not have any currently pending, so your argument about not implementing it before it's necessary is valid.
That being said, this code was already implemented elsewhere for a very similar use case so it was adopted nearly as-is. The presumption is that we'd need the currently unused functionality as we add more vendor utilities as we've seen in ironlib.
There was a problem hiding this comment.
im not familiar with the ironlib code base. does ironlib already have the supermicro update manager code? should we just be importing from ironlib?
I'm also confused. you say "do not have any currently pending" and "as we add more vendor utilities". How can this be both?
There was a problem hiding this comment.
Ironlib has many things, but not sum. I was referring to the Executor/FakeExecutor stuff that's used for testing/execing in ironlib.
While I do not currently have any vendor utilities in the queue, I can envision a future with supporting other vendor's utilities where this code would be reused.
There was a problem hiding this comment.
should we just be importing from ironlib?
ironlib is meant to execute vendor utilities in an OS running on the host.
bmclib is meant to execute actions/calls remotely and so I think this fits in here
There was a problem hiding this comment.
If we plan to continue using this code over the longer term vs something off the shelf as suggested by @DoctorVin , I'd argue that this should be extracted into an external package that both projects could import, but we haven't finalized those plans one way or the other at this point.
…ate mananger(sum)
splaspood
dismissed
DoctorVin’s stale review
Will evaluate using an alternate mocking tool for both ironlib and bmclib in the future.
What does this PR implement/change/remove?
BIOS config management support for Supermicro (SMC) platforms implemented as a wrapper over Supermicro Update Manager (SUM).
Description for changelog/release notes
BIOS config management support for Supermicro (SMC) platforms.