-
-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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
Skip checksum when download file failed. #3079
base: master
Are you sure you want to change the base?
Conversation
This comment was marked as spam.
This comment was marked as spam.
@Buguin are you still interested in completing this PR? |
Sorry,I'm too busy working this time. Recently, I will make a push to solve the problem of not being able to return when an error is reported in the following code.
|
I have transformed nvm_download_artifact function, which allows you to determine the download status based on the execution results. |
nvm.sh
Outdated
nvm_err "Downloading ${TARBALL_URL}..." | ||
nvm_download -L -C - "${PROGRESS_BAR}" "${TARBALL_URL}" -o "${TARBALL}" || ( | ||
download_result=$(nvm_download_v2 -L -C - "${PROGRESS_BAR}" "${TARBALL_URL}" -o "${TARBALL}") | ||
nvm_err "Download Http Status ${download_result}" |
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 think we don't want to print this in the common success case, and probably not even in the error case. You can, however, wrap it in a NVM_DEBUG check for debugging:
nvm_err "Download Http Status ${download_result}" | |
if [ "${NVM_DEBUG-}" = 1 ]; then | |
nvm_err "Download Http Status ${download_result}" | |
fi |
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.
This is looking great! Can you come up with some unit tests (one that mocks out nvm_download and tests nvm_download_artifact, and another that mocks out curl/wget and tests nvm_download)?
c6cfc3a
to
c20db2a
Compare
I've rebased this, but it still needs tests. @Buguin any chance you're still interesting in completing this PR? |
Fixes #3075
Check if the file is successfully downloaded