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.

Sign up for GitHub

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

Jump to bottom

Allow installation from other repository also for git method #2401

Merged
merged 1 commit into from
Jan 15, 2021
Merged

Allow installation from other repository also for git method #2401

merged 1 commit into from
Jan 15, 2021

Conversation

nmarghetti
Copy link
Contributor

No description provided.

@nmarghetti nmarghetti force-pushed the install_other_repo branch 17 times, most recently from 671eb88 to 8179915 Compare January 7, 2021 21:59
nmarghetti
nmarghetti commented Jan 7, 2021
View reviewed changes
install.sh Outdated Show resolved Hide resolved
@nmarghetti nmarghetti force-pushed the install_other_repo branch 3 times, most recently from 185ea9d to 7bedc72 Compare January 8, 2021 17:59
ljharb
ljharb requested changes Jan 8, 2021
View reviewed changes
install.sh Outdated Show resolved Hide resolved
@nmarghetti nmarghetti force-pushed the install_other_repo branch 3 times, most recently from ffaf613 to 664fccb Compare January 8, 2021 18:10
@nmarghetti
Copy link
Contributor Author

I might still have to do some change to allow to clone from the ref branch of a PR to make it work for my other PR for Windows.

@nmarghetti nmarghetti force-pushed the install_other_repo branch 3 times, most recently from 62525c8 to d289fcc Compare January 8, 2021 19:00
@ljharb ljharb marked this pull request as draft January 8, 2021 19:07
@nmarghetti nmarghetti force-pushed the install_other_repo branch 2 times, most recently from beb3715 to faf46f5 Compare January 8, 2021 20:52
@nmarghetti nmarghetti requested a review from ljharb January 8, 2021 22:38
@nmarghetti nmarghetti marked this pull request as ready for review January 8, 2021 22:38
@nmarghetti nmarghetti force-pushed the install_other_repo branch 5 times, most recently from 5a41060 to 1dadac8 Compare January 11, 2021 22:14
@ljharb ljharb mentioned this pull request Jan 11, 2021
Merged
@ljharb ljharb marked this pull request as draft January 12, 2021 00:45
@nmarghetti nmarghetti force-pushed the install_other_repo branch 7 times, most recently from 186c8a2 to 7f23340 Compare January 13, 2021 00:56
@nmarghetti nmarghetti marked this pull request as ready for review January 13, 2021 01:04
@nmarghetti
Copy link
Contributor Author

@ljharb After digging further git functionalities, I think I managed to make it even more simple. Finally ready for final review.

@nmarghetti
Copy link
Contributor Author

Just adding the use of nvm_grep, I just saw you fixed that.

@nmarghetti
Copy link
Contributor Author

@ljharb does it look good now ?

ljharb
ljharb approved these changes Jan 15, 2021
View reviewed changes
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.

LGTM, thanks

@ljharb ljharb added the installing nvm Problems installing nvm itself label Jan 15, 2021
@ljharb ljharb merged commit 502089a into nvm-sh:master Jan 15, 2021
@sladyn98
Copy link
Contributor

Hey @nmarghetti I see some of the test arguments being hardcoded here, could you help me understand a few things
a) What is the purpose of the avoid ref
b) Shouldn't these tags be variable instead of harcoded

changeset="3abb98124e8d30c9652976c9d34a7380036083b5"
test_install "$changeset" "with changeset $changeset" "" "$changeset" "$(nvm_latest_version)"

# Handle given tag
test_install "v0.37.0" "version v0.37.0" "v0.37.0" "4054bd70cedb9998015c2d8cc468c818c7d2f57d" "$(nvm_latest_version)"

# Handle given ref
test_install "refs/pull/2397/head" "with refs/pull/2397/head" "" "9849bf494d50e74aa810451fb1f83208b0092dd6" "$(nvm_latest_version)"

CC @ljharb The last test here is causing master to fail since the v0.38.0 is same as master

test_install "master" "master" "" "" "$(nvm_latest_version)"

@ljharb ljharb mentioned this pull request Apr 16, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljharb ljharb ljharb approved these changes

Assignees
No one assigned
Labels
installing nvm Problems installing nvm itself
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@nmarghetti @ljharb @sladyn98