Skip to content
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

bypass aliased curl #2932

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

bypass aliased curl #2932

wants to merge 1 commit into from

Conversation

ryenus
Copy link
Contributor

@ryenus ryenus commented Nov 2, 2022

Use command curl add nvm_curl so that we can bypass aliased curl.

Context: #2923 (comment)

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

With these changes, there's only two places nvm_curl is needed, which suggests that inlining command may be a better choice?

if we keep nvm_curl, we need to remember to unset it in deactivate. also, there may be tests mocking curl that need to now mock nvm_curl.

nvm.sh Outdated Show resolved Hide resolved
nvm.sh Outdated Show resolved Hide resolved
nvm.sh Outdated Show resolved Hide resolved
nvm.sh Outdated Show resolved Hide resolved
nvm.sh Outdated Show resolved Hide resolved
@ryenus
Copy link
Contributor Author

ryenus commented Nov 2, 2022

With these changes, there's only two places nvm_curl is needed, which suggests that inlining command may be a better choice?

if we keep nvm_curl, we need to remember to unset it in deactivate. also, there may be tests mocking curl that need to now mock nvm_curl.

Indeed, with only 2 places need the -q option I agree it's simpler to avoid the nvm_curl function, which also make testing less complicated. I've updated the PR accordingly.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Testa are failing on master, but I’d still expect this change to cause more tests to fail, due to their method of mocking curl being defeated by the PR’s change itself.

@ryenus
Copy link
Contributor Author

ryenus commented Nov 2, 2022

Testa are failing on master

Looking at the failed test here: https://app.travis-ci.com/github/nvm-sh/nvm/jobs/586754000

✗ nvm_list_aliases works with LTS aliases
expected lts/gallium, got >lts/hydrogen<

I believe it's because of outdated test data. hopefully it can be fixed by #2933

@ljharb
Copy link
Member

ljharb commented Nov 2, 2022

Unfortunately it’s much more difficult than that; xenial can’t build node 18 and newer Ubuntus can’t build node 0.6.

@jordyjwilliams
Copy link

Checked, all looks as if will work as expected and resolves #2923

@jordyjwilliams
Copy link

Running the edited command curl for me does not log the edited one.

@ljharb
Copy link
Member

ljharb commented Dec 23, 2022

Got tests passing on master and rebased this.

@ZackaryJacobthereal

This comment was marked as spam.

@ljharb
Copy link
Member

ljharb commented Jan 9, 2023

@ryenus tests are still failing - specifically the ones that mock curl. are you interested in completing this PR? if not, please leave it open so I (or another contributor) can give it a shot :-)

@ryenus
Copy link
Contributor Author

ryenus commented Jan 10, 2023

@ljharb I just realized that the test for nvm_curl_libz_support assumes that curl can be redefined as a shell function, therefore I'm keeping it as-is.

@ljharb
Copy link
Member

ljharb commented Jan 10, 2023

@ryenus indeed - so i think that with this PR's change, to keep that test passing, we'd have to make the test instead mock out the PATH and provide an actual fake curl binary.

Meanwhile keeping nvm_curl_version and nvm_curl_libz_support as-is
to avoid breaking tests which redefines `curl` as a shell function.
@@ -101,7 +101,7 @@ nvm_get_latest() {
if nvm_curl_use_compression; then
CURL_COMPRESSED_FLAG="--compressed"
fi
NVM_LATEST_URL="$(curl ${CURL_COMPRESSED_FLAG:-} -q -w "%{url_effective}\\n" -L -s -S https://latest.nvm.sh -o /dev/null)"
NVM_LATEST_URL="$(command curl ${CURL_COMPRESSED_FLAG:-} -q -w "%{url_effective}\\n" -L -s -S https://latest.nvm.sh -o /dev/null)"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This causes https://app.travis-ci.com/github/nvm-sh/nvm/jobs/592909538#L547

    nvm_get_latest
    ✗ nvm_get_latest
success path did not return version 'v12.3.456', got 'v0.39.3'
    ✗ nvm_get_latest failed redirect
failed redirect did not report correct error message, got 'v0.39.3'

@ryenus
Copy link
Contributor Author

ryenus commented Jan 10, 2023

so i think that with this PR's change, to keep that test passing, we'd have to make the test instead mock out the PATH and provide an actual fake curl binary.

@ljharb you're right, though IMHO it's a bit too much, as I think in the original issue it's likely a problem with the specific alias definition, not that the curl command cannot be aliased. If that's fine I'd like to close the pr instead.

@ljharb
Copy link
Member

ljharb commented Jan 10, 2023

Let's keep it open; I'll try to get back to it and finish it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants