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

Fixed the NVM Installer #361

Merged
merged 1 commit into from
Aug 12, 2014
Merged

Fixed the NVM Installer #361

merged 1 commit into from
Aug 12, 2014

Conversation

mohitmamoria
Copy link
Contributor

The NVM installer was using 'echo' statement when echoing the string with '\n' in it. 'echo' statement doesn't work with '\n' and thus we have to use printf command. The official installer does use 'printf' itself (link to official).

This pull request fixes the issue.

@Ilyes512
Copy link
Contributor

Ilyes512 commented Aug 9, 2014

Either printf or echo -e will do 👍

@fideloper
Copy link
Owner

this is good to merge, right @Ilyes512. I think you said you had some fixes to do?

@Ilyes512
Copy link
Contributor

I handt had a chance to fix it yet. The node installer doesn't work at the moment, right? Cause of the version "grepping". I will be having a look at it today, but probably tomorrow.

@fideloper
Copy link
Owner

Yea, that's the issue so far as know. I think the fix is as simple as this:

Changing this line to this seemed to work for me in bash:

curl 'nodejs.org' | grep 'Current Version' | awk '{ print $4 }' | awk -F\< '{ print $1 }'

Where in I print $4 rather than print $3. Also grabs the string with the v in it.

(That's referencing Vaprobash12 Issue 3)

@Ilyes512
Copy link
Contributor

If you tested it and it works ^^ So the version prefixed with v is no problem?

@fideloper
Copy link
Owner

I only did the curl request adjustment, haven't tested full nvm install.

Not sure about the v, I imagine it would be an issue.

On Tuesday, August 12, 2014, Ilyes [email protected] wrote:

If you tested it and it works ^^ So the version prefixed with v is no
problem?


Reply to this email directly or view it on GitHub
#361 (comment).

@mohitmamoria
Copy link
Contributor Author

It works fine. I am using it to provision my production servers, and they
were provisioned nicely. :)

  • Mohit :)

On Tue, Aug 12, 2014 at 4:35 PM, Chris Fidao [email protected]
wrote:

I only did the curl request adjustment, haven't tested full nvm install.

Not sure about the v, I imagine it would be an issue.

On Tuesday, August 12, 2014, Ilyes [email protected] wrote:

If you tested it and it works ^^ So the version prefixed with v is no
problem?


Reply to this email directly or view it on GitHub
#361 (comment).


Reply to this email directly or view it on GitHub
#361 (comment).

fideloper added a commit that referenced this pull request Aug 12, 2014
@fideloper fideloper merged commit 1b2aa15 into fideloper:master Aug 12, 2014
fideloper added a commit that referenced this pull request Aug 12, 2014
@fideloper
Copy link
Owner

I updated the curl request to get $4, which grabs the version with the "v". I believe that's correct as the distributions have the "v" in the filename, so we should be good to go now 👍

DanSmith70 pushed a commit to Ayima/Vaprobash that referenced this pull request Aug 22, 2014
* commit 'ced20163d501852c35757014080a7fc170e1a806':
  changing version for release
  'fixing' an addition to fideloper#361 to get the latest version of Node via their site
  naming vm machines vaprobash
  moving server timezone provisioning with rest of provisioners
  moving to more solid use case of proxying to php-fpm from apache
  fix wrong argument being passed to sed for setting php date.timezone
  distinction between server_timezone and php_timezone
  Adds zeromq.sh script to the Vagrantfile
  updated readme for contrib notes
  update to contrib docs
  Adds ØMQ script
  Added MongoDB remote access functionality.
  Update Vagrantfile
  fixed the nvm installer
  Update Vagrantfile
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.

3 participants