bmclib version 2 #280

joelrebel · 2022-06-17T16:41:59Z

This version is where all features and fixes for the newer client interface methods are to be implemented going ahead.

Clients are to import this version as github.com/bmc-toolbox/bmclib/v2.

I'm looking for suggestions on if this should be a separate v2 branch instead of a directory.

The older interface methods will continue to be maintained as is.

The only difference as of now with the previous bmclib release
is that the Device object type returned by the Inventory() methods are
of the github.com/bmc-toolbox/common.Device type.

This version is where are features and fixes are to be implemented going ahead. The only difference as of now with the previous bmclib release is that the Device object type returned by the Inventory() methods are of the github.com/bmc-toolbox/common.Device type.

codecov · 2022-06-17T16:44:10Z

Codecov Report

Merging #280 (ba4c8c3) into master (06ad9d6) will increase coverage by 18.49%.
The diff coverage is 75.55%.

❗ Current head ba4c8c3 differs from pull request most recent head fb7d32a. Consider uploading reports for the commit fb7d32a to get more accurate results

@@             Coverage Diff             @@
##           master     #280       +/-   ##
===========================================
+ Coverage   23.88%   42.37%   +18.49%     
===========================================
  Files          70       26       -44     
  Lines       11457     2329     -9128     
===========================================
- Hits         2736      987     -1749     
+ Misses       8126     1221     -6905     
+ Partials      595      121      -474

Impacted Files	Coverage Δ
bmc/firmware.go	`95.00% <ø> (ø)`
bmc/postcode.go	`95.00% <ø> (ø)`
client.go	`0.00% <0.00%> (ø)`
providers/asrockrack/asrockrack.go	`44.89% <0.00%> (ø)`
providers/asrockrack/firmware.go	`10.18% <0.00%> (ø)`
providers/asrockrack/helpers.go	`47.58% <ø> (ø)`
providers/asrockrack/power.go	`0.00% <ø> (ø)`
providers/asrockrack/user.go	`85.89% <ø> (ø)`
providers/ipmitool/ipmitool.go	`0.00% <ø> (ø)`
providers/redfish/firmware.go	`43.44% <ø> (ø)`
... and 15 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 06ad9d6...fb7d32a. Read the comment docs.

jacobweinstock · 2022-06-22T12:44:55Z

Hey @joelrebel , curious if there are some specific challenges with the current code structure this is seeking to solve? I don't believe versioning changes with this new code structure, right?

joelrebel · 2022-06-22T14:08:55Z

Hey @jacobweinstock, thanks for taking a look, theres a few reasons for the suggested v2, since raising this PR, I realize a branch v2 is recommended over a directory v2.

The thought process behind the v2 is,

The older ScanAndConnect and related interfaces will stay in maintenance mode, imported by clients as github.com/bmc-toolbox/bmclib
The newer client and related interfaces gets new hardware support and features, the v2 release will not include the older client and interface methods, imported by clients as github.com/bmc-toolbox/bmclib/v2

At the moment, theres one breaking change to the newer client Inventory() methods, where the device object returned
is now of the common.Device type.

Apart from the this,

a. I'd like bmclib to have one client interface that clients can use (instead of two) v2 gives us that way ahead.

b. The older interface methods get pretty much zero new hardware support and have limited maintenance being done. Its hardware compatibility with older interface methods is questionable, from what I see, a lot of the older interface methods do not work with newer Dells and Supermicros, and so I'd like to get rid of code that is not in use.

d. I'd like to also add support for newer hardware without wading through all the cruft of the older unmaintained codebase and working around conflicts in the method and variable names - which shows up when I'm adding new hardware support for the newer client.

c. In v2 the newer client methods can also be adapted to use Go Generics, reducing quite a bit of boilerplate code there.

The choice of v2 as the branch name - given that there is no v1 is because Go expects a major version release that is not on the master branch to be v2 and higher, along with the idea is that clients explicitly import github.com/bmc-toolbox/bmclib/v2 so that they do not end up upgrading by mistake.

bmclib should have been tagged a v1.x.x a while ago, but we have been conservative in the tagging and so I'm inclined to jump straight to v2 with these set of changes.

jacobweinstock · 2022-06-22T14:20:34Z

This is great! Thanks for sharing this reasoning.

What makes you say a v2 branch is preferred over a v2 directory? From this post, I believe the v2 directory is preferred, no?

joelrebel · 2022-06-22T14:40:24Z

Based on what I understood from https://github.com/golang/go/wiki/Modules#releasing-modules-v2-or-higher
having the v2.*.* tags on a separate v2 branch reduces confusion to some extent.

The other reason was that a v2 directory would require separate CI, linter related configuration while they sit on the same branch. I'm still open to a v2 directory if that makes more sense/is easier to work with in this case.

chrisdoherty4 · 2022-07-05T15:00:29Z

FWIW, I advocate branches over sub-directories as I've found it much easier to maintain over a sub-directory with respect to how Go modules work and git workflows like cherry-picking fixes.

mmlb

I only made it about 1/3 of the way so far but now I'm off for a while so might as well add what I've got.

v2/bmc/boot_device.go

mmlb · 2022-07-14T16:19:32Z

v2/bmc/firmware.go

+}
+
+// FirmwareInstallStatusFromInterfaces pass through to library function
+func FirmwareInstallStatusFromInterfaces(ctx context.Context, installVersion, component, taskID string, generic []interface{}) (status string, metadata Metadata, err error) {


Why take genereic []interface{} instead of []FirmwareInstallVerified?

The method is invoked with GetDriverInterfaces which returns various interfaces that may or may not implement the FirmwareInstallVerfier interface.

mmlb · 2022-07-14T16:22:20Z

v2/bmc/inventory.go

+
+	"github.com/hashicorp/go-multierror"
+	"github.com/pkg/errors"
+


remove the empty line?

mmlb · 2022-07-14T16:43:19Z

v2/bmc/power.go

+	// "reset": soft down and then power on. simulates a reboot from the host OS.
+	// "cycle": hard power down followed by a power on. simulates pressing a power button


it would be nice to specify what happens if the machine is already powered off when these come in

v2/bmc/power.go

v2/bmc/user.go

…b/tree/master/examples/v1

…olbox/common package

joelrebel · 2022-07-26T09:56:01Z

Thanks for all the comments and suggestions here, they have been incorporated into the latest set of commits.
The v2 code base is now smaller and cleaner which should allow quicker addition of new features and fixes.

Since all of this code is currently functional and running in production, I'm going to leave the remaining suggestions as is, with issues added for some.

I'll now close this PR since the base branch is master. The PR branch v2 will stay and be tagged with v2.0.0.

https://github.com/bmc-toolbox/bmclib#versions bmc-toolbox/bmclib#280 (comment)

https://github.com/bmc-toolbox/bmclib#versions bmc-toolbox/bmclib#280 (comment) Signed-off-by: Joel Rebello <[email protected]>

bmclib version 2

c42965d

This version is where are features and fixes are to be implemented going ahead. The only difference as of now with the previous bmclib release is that the Device object type returned by the Inventory() methods are of the github.com/bmc-toolbox/common.Device type.

joelrebel requested review from diogomatsubara, jacobweinstock and micahhausler June 17, 2022 16:42

joelrebel mentioned this pull request Jul 15, 2022

Implements all of the Alloy components metal-toolbox/alloy#4

Merged

4 tasks

mmlb reviewed Jul 15, 2022

View reviewed changes

joelrebel added 12 commits July 26, 2022 10:50

bmc: clearer comments on methods and unexport internal methods

ded545e

Add constants

742da04

Add examples updated for v2 from https://github.com/bmc-toolbox/bmcli…

78f8c03

…b/tree/master/examples/v1

purge v2 directory, since its going to be released as branch

7831f55

client: update imports for v2

518a2b0

purge older bmclib files

1a689b3

providers: updated to use consts and types from the github.com/bmc-to…

e884fdd

…olbox/common package

internal/utils: purge unused methods

ddd4b42

errors: purge unused older errors defs

af7f6ec

go: tidy deps and switch to 1.18

9f4e89d

doc: update doc -squash

a040782

Update golangci linter, GH workflow, Makefile, mergify configurations

fb7d32a

joelrebel force-pushed the v2 branch from ba4c8c3 to fb7d32a Compare July 26, 2022 09:49

joelrebel closed this Jul 26, 2022

joelrebel added a commit to joelrebel/pbnj that referenced this pull request Jan 23, 2023

updates bmclib dependency to v2 latest

a746552

https://github.com/bmc-toolbox/bmclib#versions bmc-toolbox/bmclib#280 (comment)

joelrebel mentioned this pull request Jan 23, 2023

updates bmclib dependency to v2 latest tinkerbell/pbnj#137

Merged

3 tasks

chrisdoherty4 pushed a commit to joelrebel/pbnj that referenced this pull request Jan 23, 2023

updates bmclib dependency to v2 latest

35a6a1c

https://github.com/bmc-toolbox/bmclib#versions bmc-toolbox/bmclib#280 (comment)

joelrebel added a commit to joelrebel/pbnj that referenced this pull request Jan 23, 2023

updates bmclib dependency to v2 latest

ee702ae

https://github.com/bmc-toolbox/bmclib#versions bmc-toolbox/bmclib#280 (comment) Signed-off-by: Joel Rebello <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bmclib version 2 #280

bmclib version 2 #280

joelrebel commented Jun 17, 2022 •

edited

Loading

codecov bot commented Jun 17, 2022 •

edited

Loading

jacobweinstock commented Jun 22, 2022

joelrebel commented Jun 22, 2022 •

edited

Loading

jacobweinstock commented Jun 22, 2022 •

edited

Loading

joelrebel commented Jun 22, 2022

chrisdoherty4 commented Jul 5, 2022 •

edited

Loading

mmlb left a comment

mmlb Jul 14, 2022

joelrebel Jul 26, 2022

mmlb Jul 14, 2022

mmlb Jul 14, 2022

joelrebel commented Jul 26, 2022

		// "reset": soft down and then power on. simulates a reboot from the host OS.
		// "cycle": hard power down followed by a power on. simulates pressing a power button

bmclib version 2 #280

bmclib version 2 #280

Conversation

joelrebel commented Jun 17, 2022 • edited Loading

codecov bot commented Jun 17, 2022 • edited Loading

Codecov Report

jacobweinstock commented Jun 22, 2022

joelrebel commented Jun 22, 2022 • edited Loading

jacobweinstock commented Jun 22, 2022 • edited Loading

joelrebel commented Jun 22, 2022

chrisdoherty4 commented Jul 5, 2022 • edited Loading

mmlb left a comment

Choose a reason for hiding this comment

mmlb Jul 14, 2022

Choose a reason for hiding this comment

joelrebel Jul 26, 2022

Choose a reason for hiding this comment

mmlb Jul 14, 2022

Choose a reason for hiding this comment

mmlb Jul 14, 2022

Choose a reason for hiding this comment

joelrebel commented Jul 26, 2022

joelrebel commented Jun 17, 2022 •

edited

Loading

codecov bot commented Jun 17, 2022 •

edited

Loading

joelrebel commented Jun 22, 2022 •

edited

Loading

jacobweinstock commented Jun 22, 2022 •

edited

Loading

chrisdoherty4 commented Jul 5, 2022 •

edited

Loading