Add a download failure message, rather than trying to unzip an HTML page #24
Conversation
subdigital
force-pushed
the
download-failure-error-msg
branch
from
647a7dc to
f5aafae
Compare
|
I have signed the CLA |
There was a problem hiding this comment.
Thanks, @subdigital! Definitely makes the error message more helpful. Left some questions.
| let (localURL, _) = try await URLSession.shared.download(from: url, delegate: self) | ||
| let (localURL, response) = try await URLSession.shared.download(from: url, delegate: self) | ||
| let statusCode = (response as? HTTPURLResponse)?.statusCode ?? 0 | ||
| if statusCode != 200 { |
There was a problem hiding this comment.
Does this assumption stand for URLs that redirect? I haven't encountered a redirecting URL for any artifacts I've worked with, but being mindful of others that may be relying on this today.
Also, could potentially turn this into a guard.
There was a problem hiding this comment.
Redirects are handled implicitly by URLSession, unless you intervene via the delegate.
| let (localURL, response) = try await URLSession.shared.download(from: url, delegate: self) | ||
| let statusCode = (response as? HTTPURLResponse)?.statusCode ?? 0 | ||
| if statusCode != 200 { | ||
| let body = (try? String(contentsOf: localURL)) ?? "<no response>" |
There was a problem hiding this comment.
How valuable is the actual response data? It does seem to clutter the alert quite a bit—is the status code enough? To get more granular I wonder if it could be enough to look at the logs via Console. But let me know what you think.
There was a problem hiding this comment.
I know what you mean... I can look at sending this to the console and instead showing a generic error for HTTP 403 or 401
|
Hey @subdigital, apologies for pulling the rug out from under these changes! I spent the last couple of weeks making some architectural changes to Tophat. HTTP downloads are now handled by |
Developers have to change the team ids in order to build and then have to remember to revert these changes before submitting a PR. This moves the definition of DEVELOPMENT_TEAM into a config file that is gitignored. A bootstrap script is provided to make getting set up easier.
bootstrap.sh is now setup_team.sh and can take a 'default' argument. Xcode will call this script if the xcconfig file is not present on disk, making it a zero-setup process for internal contributors. External contributors will need to run setup_team.sh once to set up their own team id.
subdigital
force-pushed
the
download-failure-error-msg
branch
from
2a995b3 to
efe3c0e
Compare
|
This PR Builds on top of #59 and re-applies the download error behavior for the new extension system. To do this I had to move the error enum up a level to I also got some weird linker errors about using Logger so I stuck with |
The new alert looks like this |
I don't think this is needed. Any
Yes, that's expected. The logger is unavailable in extensions. We could use |
|
Hi @subdigital, do you still plan to work on this? |
What does this change accomplish?
If you try to install a build where the URL it downloads gives you an HTTP 403 response, previously Tophat would ignore the status code and save the resulting response body to the target file. Later it would get a failure trying to unzip this file.
How have you achieved it?
Tophat now looks for a 200 response status code, and if it is not, it reports it as an error. The failure reason is attached to the the error enum so that we can display it to the user.
How can the change be tested?
Open up an install URL to a non-existent AWS S3 bucket:
http://localhost:29070/install/ios?virtual=https://some-bucket-we-dont-have-access-to.s3.us-east-1.amazonaws.com/foo/build.app.zip
See the error like this: