Consult http_code and download_error_msg only for failed downloads. #137
Conversation
|
|
|
@bozturkMSFT could you please review? |
|
|
||
| # if the download fails, download the legacy_download_link | ||
| if [ "$download_failed" = true ]; then | ||
| primary_path_http_code="$http_code"; primary_path_download_error_msg="$download_error_msg" |
There was a problem hiding this comment.
This will still be unbound if you don't have wget or curl - download will fail, but you won't have an error code. @bozturkMSFT does it make sense to say, just exit the script if neither was found?
There was a problem hiding this comment.
That sounds reasonable enough to me at least so I updated the PR to do exactly that.
There was a problem hiding this comment.
Returning 1 from download() will sadly preserve this error behavior. Exiting directly from that function wont .
There's two options:
- Just log the message there and exit. There's little that can be done at any other point in the script that's useful to the users. This also avoids doing retries if there's no curl or wget.
- Change this to something like:
| primary_path_http_code="$http_code"; primary_path_download_error_msg="$download_error_msg" | |
| primary_path_http_code="${http_code:-toolError}"; primary_path_download_error_msg="$download_error_msg" |
The messages will repeat and do retries, but it's pretty small of a fix.
Even with this contribution I'd be happy, as most error cases in the script will be "couldn't download", not "couldn't find something to download with", and this would fix issues that are pretty disruptive right now.
| unset http_code | ||
| download_error_msg="Missing dependency: neither curl nor wget was found." | ||
| break | ||
| say_err "Missing dependency: neither curl nor wget was found." |
There was a problem hiding this comment.
I'd recommend that you return to the old logic here. There should be no http_code if there's no http request.
There was a problem hiding this comment.
Do you mean that I should be unsetting it? We are exiting the script when we hit this condition though, so that doesn't matter unless I am missing something? http_code is only a global and not an env var.
FWIW it appears that the script was already exiting if neither curl or wget were present thanks to check_min_reqs().
There was a problem hiding this comment.
Ah, then maybe this is defensive. return 1 wouldn't exit here, but if check_min_reqs does it, then your original fix was better. Just means that the no-wget-or-curl branch of this was dead all along. Thanks for explaining.
There was a problem hiding this comment.
Yep, I missed that about returning 1 from inside the function not actually exiting, thank you for catching that. I think it makes sense to be defensive also even if the branch is dead, and I think it's better to just exit with the message logged so I updated the PR to that fwiw.
|
Just to reiterate this has stopped all our deployments and a lot of development work, we rely on roslyn in production, please can the fix be merged ASAP or the latest install scripts be rolled back to yesterday. |
Should fix #136