feat: Add methods blink, update and device to supervisor v1 struct #79
Conversation
cakoolen
force-pushed
the
Add-update-endpoint-to-supervisor-v1-service
branch
2 times, most recently
from
a9bf476 to
3589bdb
Compare
| @@ -29,22 +30,104 @@ func (s *SupervisorV1Service) Reboot(ctx context.Context, force bool) error { | |||
| &request{Force: force}, | |||
| ) | |||
| if err != nil { | |||
| return fmt.Errorf("unable to create restart service request: %v", err) | |||
| return fmt.Errorf("unable to create restart service request: %w", err) | |||
There was a problem hiding this comment.
What's the reason for this change? As this is an open source library, I'd be reluctant to exposing the underlying implementation error without a good reason.
There was a problem hiding this comment.
I can understand that though we probably want to respond differently when we receive an internal server error versus a bad request response. If we can wrap this error here, on a higher level we can decide whether to do a retry or ask an operator for help.
supervisorv1.go
Outdated
| return fmt.Errorf("unable to create blink request: %w", err) | ||
| } | ||
|
|
||
| err = s.client.Do(req, io.Discard) |
There was a problem hiding this comment.
We should be able to switch to (also in other places)
| err = s.client.Do(req, io.Discard) | |
| err = s.client.Do(req, nil) |
supervisorv1.go
Outdated
| response := new(SupervisorV1DeviceResponse) | ||
|
|
||
| err = s.client.Do(req, response) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("unable to get device state: %w", err) | ||
| } | ||
|
|
There was a problem hiding this comment.
We generally follow the slightly more idiomatic form (at least these days)
| response := new(SupervisorV1DeviceResponse) | |
| err = s.client.Do(req, response) | |
| if err != nil { | |
| return nil, fmt.Errorf("unable to get device state: %w", err) | |
| } | |
| var response SupervisorV1DeviceResponse | |
| err = s.client.Do(req, &response) | |
| if err != nil { | |
| return nil, fmt.Errorf("unable to get device state: %w", err) | |
| } | |
There was a problem hiding this comment.
I'm fine with that. Though in other parts of this repository, I see a construction where the response created by:
response := &SupervisorV1DeviceResponse{}
In hindsight, I probably should have used that. Do you still want me to stick to your suggestion?
There was a problem hiding this comment.
Yeah the library needs a little cleaning to be honest. Lets go with the more idiomatic way I suggested here for now
There was a problem hiding this comment.
Thanks. That will give me a guide for potential future pull requests.
cakoolen
force-pushed
the
Add-update-endpoint-to-supervisor-v1-service
branch
from
3589bdb to
df029f7
Compare
Added the following methods: - Blink: start a blink pattern on a LED for 15 s - Update: trigger a check for the target state of configurations - Device: return the current device state
cakoolen
force-pushed
the
Add-update-endpoint-to-supervisor-v1-service
branch
from
df029f7 to
14563d1
Compare
Added the following methods: