search

tinkerbell / pbnj

Service for interacting with BMCs
Apache License 2.0
109 stars 37 forks source link

Add screenshot service, devcontainer. #144

Closed mattcburns closed 1 year ago

mattcburns commented 1 year ago

Description

The primary feature of this PR is to add screenshot support to the service that was earlier added to bmclib. In addition, I have added devcontainer support to make development easier on some platforms.

I have removed go-grpc validations from the protobuf definitions and grpc build process. This is due to incompatibility with building grpc libraries for languages other than Go. If there is some workaround (or I missed something), I am open to suggestions but otherwise it should be removed so other languages can use the client generation capabilities.

How Has This Been Tested?

I have added a command line option under a new command subcategory called diagnostic screenshot to test capture of the screenshots via CLI. A user needs to set the BMCIP and BMCPASS environment variables and the command line tool will use these to connect to the target machine.

How are existing users impacted? What migration steps/scripts do we need?

No major impact to end users - if they want to take advantage of the new options they will need to update any tooling around this application.

Checklist:

I have:

jacobweinstock commented 1 year ago

Hey @mattcburns, thanks for the PR! I'm not convinced that adding this amount of Ruby code is a good idea for the repo. The existing Ruby code was only there as a very basic example of a client. This amount of code feels like it's a library and now something we'll need to maintain. I'm not familiar enough with Ruby to want to take this on. I don't believe the other maintainers are familiar with Ruby either but i'll let them chime in. cc @joelrebel , @chrisdoherty4

mattcburns commented 1 year ago

Hey @mattcburns, thanks for the PR! I'm not convinced that adding this amount of Ruby code is a good idea for the repo. The existing Ruby code was only there as a very basic example of a client. This amount of code feels like it's a library and now something we'll need to maintain. I'm not familiar enough with Ruby to want to take this on. I don't believe the other maintainers are familiar with Ruby either but i'll let them chime in. cc @joelrebel , @chrisdoherty4

I can just remove it, it's auto generated from the protobufs so we don't really need to keep it in there. Thanks for the feedback!

mattcburns commented 1 year ago

I addressed all the feedback - I didn't realize that a force push after I rebased would remove most of those inline comments, so I apologize for that, but I think they're all still part of the conversation view. Thank you!

mattcburns commented 1 year ago

Addressed the additional feedback - this should be ready for further review @jacobweinstock @joelrebel Thanks!