intelamt provider interface

@jacobweinstock In a recent commit I broke a intelamt test https://github.com/bmc-toolbox/bmclib/actions/runs/3856309968/jobs/6572367721#step:4:403

To fix this, Im considering extending the amtProvider interface to include a IsNil() method.

Then the Compatible() method can check if the instance is IsNil(), and will enable a mock instance to be passed in like you have here https://github.com/bmc-toolbox/bmclib/blob/main/providers/intelamt/intelamt.go#L110.

What are your thoughts on this?

Apologies, i ran through this PR too fast. Why is this needed amtclient, ok := c.client.(*amt.Client)? Couldn't a direct check of c.client == nil be used?

This is already done here:

bmclib/providers/intelamt/intelamt.go

Line 85 in 828737c

if c.client == nil {

The nil check fails to match on the nil interface, and so it needs to be type asserted and checked
*amt.Client(<nil>) != nil basically, unless I'm looking at it wrong..

Hey Joel, sorry for the delayed response. I don't know if i following here. This code snippet below shows that a nil check on a nil interface does indeed work. Maybe I don't understand what you're saying, sorry.

https://goplay.tools/snippet/xQou00yUQfZ

https://goplay.tools/snippet/FLSB4vFi6yX

Interfaces have a pointer to another construct, that construct could also be a pointer. The i == nil check only validates the internal interface pointer is not nil. If the internal interface pointer points to another nil pointer and you try to use its methods, it will panic.

I've not experienced this particular problem, but I also tend to avoid type asserting.

Edit: updated with a simplified example.

I took a deeper look at the code and I'm curious if we need 2 ways to initialize intelamt.Conn: via New and via Open having constructed the object. I'm guessing Open satisfies some interface? Can we drop Open as it would remove the need for .client == nil checks?

If Open is required, the only check that's needed is .client == nil return "connection uninitialized; Open() must be called before Conn can be used". While my previous comment is true, its only relevant if some external entity can manipulate the .client field. This might be where Jacob was coming from with his previous comment. (Side note: if amt.NewConn returned nil, nil meaning no client and no error Conns methods would panic; I'd class that as an amt package bug though).

In either case, the intelamt package doesn't expose .client as a field to be modified so the intelamt package is responsible for ensuring its correctly initialized before use. Unit tests will panic if its not, which is fine because they're catching a programmer error.

Thanks for the feedback here, @jacobweinstock fix in #303 seems to work and so I'll close this issue.