New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement self-update
command
#81
Implement self-update
command
#81
Conversation
need to unify download, add tests
todo: - update existing usages of mock HTTP downloader - finish self update tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general looks good. Adding a few comments
Sources/SwiftlyCore/HTTPClient.swift
Outdated
} | ||
} | ||
|
||
private func makeRequest(url: String) -> HTTPClientRequest { | ||
var request = HTTPClientRequest(url: url) | ||
request.headers.add(name: "User-Agent", value: "swiftly") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should include the swiftly version here as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Sources/SwiftlyCore/HTTPClient.swift
Outdated
private struct Response { | ||
let status: HTTPResponseStatus | ||
let buffer: ByteBuffer | ||
} | ||
|
||
private let downloader: ToolchainDownloader | ||
private let inner: HTTPRequestExecutor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename to executor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Sources/SwiftlyCore/HTTPClient.swift
Outdated
// As a heuristic for if we've hit the 404 page, we check to see if the content is HTML. | ||
if url.host == "download.swift.org" { | ||
guard !response.headers["Content-Type"].contains(where: { $0.contains("text/html") }) else { | ||
throw SwiftlyHTTPClient.DownloadNotFoundError(url: url.path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going to ask can't you check the response status, but apparently not. The 404 page returns a status of 302. You could check the location header is https://swift.org/404.html
instead though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So after following the redirect, swift.org/404.html does in fact return a status code of 404 nowadays, so I've just gone ahead and removed this special check altogether.
Implements a
self-update
command, which can be used to update swiftly in-place.This also expands the toolchain downloading mocking framework introduced in #67 to be able to be used for any arbitrary HTTP request. This allows us to use mocking to simulate there being a swiftly update to make, even though there are none currently.
For the 0.2.0 release, users will need to install via
swiftly-install.sh
, but every release afterwards they can do it via a simpleswiftly self-update
.