Add support for CLI update command#311
Conversation
angaz
left a comment
angaz
left a comment
There was a problem hiding this comment.
This is an awesome feature! I hope it gets merged soon.
rocketpool-cli/update/update.go
Outdated
| err = os.Remove(ex) | ||
| if err != nil { | ||
| return fmt.Errorf("error while removing old rocketpool binary: %w", err) | ||
| } | ||
| err = os.Rename(fileName, ex) | ||
| if err != nil { | ||
| return fmt.Errorf("error while writing new rocketpool binary: %w", err) | ||
| } |
There was a problem hiding this comment.
os.Rename should replace the original file according to the docs: https://pkg.go.dev/os#Rename
So the os.Remove isn't needed.
There was a problem hiding this comment.
Hi @SN9NV . I based this behaviour on this Stackoverflow question, but I just tested and it does appear to work without the unlink first.
There was a problem hiding this comment.
When replacing the binary, the key is the the inode is different to the original executable. We have a new file created during the download process. So this is pretty safe. If we were using something else like truncate/copy, then we might still have the original inode, which can be problematic for the OS to deal with.
rocketpool-cli/update/update.go
Outdated
| func checkSignature(signatureUrl string, pubkeyUrl string, verification_target *os.File) error { | ||
| pubkeyResponse, err := http.Get(pubkeyUrl) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
Please add some error context.
rocketpool-cli/update/update.go
Outdated
|
|
||
| signatureResponse, err := http.Get(signatureUrl) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
Please add some error context.
rocketpool-cli/update/update.go
Outdated
| client := getHttpClientWithTimeout() | ||
| resp, err := client.Get(GithubAPIGetLatest) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
Please add some error context.
rocketpool-cli/update/update.go
Outdated
| } | ||
| body, err := io.ReadAll(resp.Body) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
Please add some error context.
rocketpool-cli/update/update.go
Outdated
| } | ||
|
|
||
| // Update the Rocket Pool CLI | ||
| func updateCLI(c *cli.Context) error { |
There was a problem hiding this comment.
I see a few main sections to this function. I think it would be easier to see the main steps if they are split into their own functions. It would also help add context to the errors. I see the same error message repeated/similar to another one. What do you think?
There was a problem hiding this comment.
Yes, I think that's a fair comment. I've split out the initial version check, and the download/verify sections into their own functions, and I think updateCLI is pretty readable now :)
|
Seems great. I tested the various flags and it seems to be working as expected. There's just these two issues from Not critical things, but I think probably good to have a look at. |
|
I've added a check for the
|
Looks good. Yeah I think it will be fine and we can replace the library if there's any CVEs. I'm pretty sure it will be perfectly fine. |
|
I rate it's good to go. 🚀 |
2 participants
As requested by Phiz in #268, I have added support for a
rocketpool update clicommand which will:There are some assumptions regarding the filenames in the release but these appear to be reasonably widely referenced already.
Optional flags included for this command:
--forcewill attempt to update, even if Github version is same/older--skip-signature-verificationwill prevent any checks against the PGP signature--yeswill automatically confirm the update