Skip to content
This repository has been archived by the owner on Aug 30, 2024. It is now read-only.

feat: Add update command to coder-cli #417

Merged
merged 26 commits into from
Aug 17, 2021
Merged

Conversation

johnstcn
Copy link
Member

@johnstcn johnstcn commented Aug 12, 2021

This commit adds a new update subcommand that queries a Coder instance for its current version, fetches the corresponding version from GitHub releases if required, and updates the binary in-place.

Currently, we only check Major.Minor.Patch version, and not pre-release. (Edit: checking pre-release now)
This will obviously not work if the corresponding release of coder-cli is not yet available.

This commit adds a new update subcommand that queries a Coder instance
for its current version, fetches the corresponding version from
GitHub releases if required, and updates the binary in-place.
@johnstcn johnstcn self-assigned this Aug 12, 2021
internal/cmd/update.go Outdated Show resolved Hide resolved
internal/cmd/update.go Outdated Show resolved Hide resolved
internal/cmd/update_test.go Outdated Show resolved Hide resolved
internal/cmd/update.go Outdated Show resolved Hide resolved
internal/cmd/update.go Outdated Show resolved Hide resolved
internal/cmd/update.go Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
go.mod Show resolved Hide resolved
internal/cmd/update.go Outdated Show resolved Hide resolved
internal/cmd/update.go Outdated Show resolved Hide resolved
internal/cmd/update.go Outdated Show resolved Hide resolved
internal/cmd/update.go Outdated Show resolved Hide resolved
@johnstcn johnstcn marked this pull request as ready for review August 16, 2021 12:50
Copy link
Member

@kylecarbs kylecarbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally and works wonderfully!

executablePath string
fs afero.Fs
httpClient getter
osF func() string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

osF can just be a string instead of a function.

Get(url string) (*http.Response, error)
}

func (u *updater) Run(ctx context.Context, force bool, coderURLArg string, versionArg string) error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function could benefit from being split up a little bit more, it's fairly long as it stands right now

internal/cmd/update.go Show resolved Hide resolved
internal/cmd/update_test.go Outdated Show resolved Hide resolved
internal/cmd/update_test.go Show resolved Hide resolved
@deansheather
Copy link
Member

Also I think for the initial version we shouldn't worry about querying github API, the release artifacts are consistent right now. This PR is getting pretty big

@johnstcn
Copy link
Member Author

Also I think for the initial version we shouldn't worry about querying github API, the release artifacts are consistent right now. This PR is getting pretty big

I found the opposite, we have coder-cli-linux-amd64.zip vs coder-cli-windows.zip or coder-cli-windows-386.zip and so on. Querying the GitHub releases API seems less failure-prone here.

Copy link
Contributor

@jawnsy jawnsy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, my comments are minor, feel free to address them in a follow-up (or not at all).

}

for _, prefix := range pathBlockList {
if HasFilePathPrefix(u.executablePath, prefix) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This behavior is good for now, just wanted to leave a suggestion for your consideration in a future iteration: we could also write the file to a well-known location (e.g. ~/.local/coder/bin) or similar, and then use LookPath to see if we find the file we just wrote. If the version doesn't match, then we know that the user has something else earlier on their path obscuring the location of where we wrote the file, so we could issue a message in that case instructing users what to do (e.g. move ~/.local/coder/bin earlier in the PATH, or remove the other one from the PATH, etc)

internal/cmd/update.go Show resolved Hide resolved
internal/cmd/update.go Outdated Show resolved Hide resolved
internal/cmd/update.go Outdated Show resolved Hide resolved
Version string `json:"version"`
}{}

body, err := ioutil.ReadAll(resp.Body)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, we should consider increasing our version requirement for coder-cli to 1.16 so that we can use io.ReadAll instead (ioutil stuff is now deprecated). No rush to do that, though, we can do it after 1.17 is released (should be soon)

internal/cmd/update.go Show resolved Hide resolved
internal/cmd/update.go Show resolved Hide resolved
@johnstcn johnstcn merged commit 59a0a20 into master Aug 17, 2021
@johnstcn johnstcn deleted the cianjohnston/autoupdate branch August 17, 2021 11:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants