Add support for nvm_verb, update readme #27

RulerOf · 2018-03-12T20:34:48Z

So this is the pull request that's just the readme updates and code changes. I think.

I assembled this manually from the full diff so I can't say I actually tested it this way. It looks right to me, though.

Edit: confirmed in #28 that this is functionally identical to #26

RulerOf · 2018-04-16T20:08:44Z

@koenpunt any thoughts on this patch?

koenpunt

Apologies for the late review. Thank you for this PR! I added some remarks, and 1 question.

koenpunt · 2018-05-20T22:27:19Z

README.md

 ```

-If your nvm is located in some custom path, you can use `nvm_custom_path` to set it.
+When NVM is installed at the system level:


NVM is supposed to be written in lowercase.

koenpunt · 2018-05-20T22:30:26Z

lib/capistrano-nvm/version.rb

@@ -1,3 +1,3 @@
 module CapistranoNvm
-  VERSION = '0.0.7'
+  VERSION = '0.0.9'.freeze


We can move to 0.1.0 I think.

Heh. I actually had to check the Semver spec before I realized that you're absolutely right about that 😆

koenpunt · 2018-05-20T22:31:23Z

README.md

+- `:nvm_map_bins`
+  - Accepts: Ruby word array
+  - Default: `%w{node npm yarn}`
+  - Notes: This is a list of commands that will be executed with NVM support by capistrano.


The nvm abbreviation is supposed to written in lowercase, Capistrano on the other hand should be capitalized.

Fixed this and the other occurrences of improper capitalization

koenpunt · 2018-05-20T22:33:01Z

README.md

+
+- `:nvm_custom_path`
+  - Accepts: String
+  - Default: Unused


"Not set" or "None" would better define the default value

koenpunt · 2018-05-20T22:38:10Z

README.md

+  download! "#{release_path}/.nvmrc", '.nvmrc'
+  SSHKit.config.default_env[:node_version] = File.read('.nvmrc').strip
+end
+```


I don't really get what this is about; you download the .nvmrc and use that for the next commands? Why should the remote version be leading?
Or is this something else and do I not understand this?

This example is to show specifically how to configure your deployment with the .nvmrc file that comes from your release code.

My use case was that our release code contains a .nvmrc file, but reading that file and configuring Capistrano with the value from it required me to download! it to the machine where the Capistrano code was executing.

I will admit that there's definitely a possibility that I'm simply Doing It Wrong™ here and just am not aware, but this seemed like the right solution to set the nvm version with the one from my deployed code.

As an additional note: I'm not 100% certain why I'm setting the :node_version symbol directly instead of setting :nvm_node. It's possible that I made a mistake when I was testing this, then kept it since it worked, but it's also possible that I tried setting :nvm_node which didn't work.... I just don't recall at this point.

Could I possibly lean on your experience with Capistrano to evaluate that?

If you'd like me to verify on my end, I can test with one of my deployment jobs but will need to take the time. Just let me know.

My use case was that our release code contains a .nvmrc file, but reading that file and configuring Capistrano with the value from it required me to download! it to the machine where the Capistrano code was executing.

If you're running the Capistrano commands from the project directory locally, the same .nvmrc fill should be already present there, right? So I think your example is only relevant for people with a non-standard setup, and for that I would rather leave it out completely.
Let me know if you think it is common use, or if I didn't understand you correctly.

If you're running the Capistrano commands from the project directory locally, the same .nvmrc fill should be already present there, right?

In our setup, the Capistrano code runs on a Jenkins server, and the release code ends up on our web nodes. Capistrano uses SSHKit to run a bunch of git commands on each of our web servers in turn to fetch precisely the branch that's been configured for deployment. As a result, the .nvmrc file that's in our application code ("release" code in Capistrano parlance) never actually ends up on the Jenkins box where the Capistrano code is running. Hence we download the .nvmrc.

I didn't build this deployment and didn't have any experience with Capistrano prior to this, so I just assumed this was normal. Is the typical deployment case for Capistrano different in some way?

The only thing I can see wrong with it is that it's an awfully redundant procedure if you were to have a very large number of nodes. Downloading the same file from all of them is silly but it would still work.

Edit: I wasn't specific about it, but our Capistrano code and the release code live in different git repos.

Backward-compatible changes were made

koenpunt · 2018-05-21T17:19:23Z

lib/capistrano/tasks/nvm.rake

      end
    end
  end

  task :map_bins do
    SSHKit.config.default_env.merge!({ node_version: "#{fetch(:nvm_node)}" })
    nvm_prefix = fetch(:nvm_prefix, -> { "#{fetch(:tmp_dir)}/#{fetch(:application)}/nvm-exec.sh" } )
+    SSHKit.config.default_env[:nvm_verb] = fetch(:nvm_verb).to_s


Now that I look at this again, there's no need to add an environment variable for nvm_verb. Below you can simply use fetch(:nvm_verb) instead of $NVM_VERB.

koenpunt · 2018-05-21T17:20:02Z

lib/capistrano/tasks/nvm.rake

@@ -28,7 +32,7 @@ namespace :nvm do
  task :wrapper do
    on release_roles(fetch(:nvm_roles)) do
      execute :mkdir, "-p", "#{fetch(:tmp_dir)}/#{fetch(:application)}/"
-      upload! StringIO.new("#!/bin/bash -e\nsource \"#{fetch(:nvm_path)}/nvm.sh\"\nnvm use $NODE_VERSION\nexec \"$@\""), "#{fetch(:tmp_dir)}/#{fetch(:application)}/nvm-exec.sh"
+      upload! StringIO.new("#!/bin/bash -e\nsource \"#{fetch(:nvm_path)}/nvm.sh\"\nnvm $NVM_VERB $NODE_VERSION\nexec \"$@\""), "#{fetch(:tmp_dir)}/#{fetch(:application)}/nvm-exec.sh"


So here you can replace $NVM_VERB with fetch(:nvm_verb)

Does the same go for $NODE_VERSION and :nvm_node?

Edit: I expect that the answer is yes, but do appreciate your expertise on this. :)

Oh crap, I guess I'm wrong here, this is of course the script that is created remotely. I'm sorry, it has been a long time that I worked on this project.

The only thing I'm now thinking is; wouldn't it be better to add a nvm:install task, and let that be idempotent, instead of having side effect when executing a node binary?

Oh crap, I guess I'm wrong here, this is of course the script that is created remotely.

Hmm... Thinking about it again, my current deploys rely on the destination servers being able to track changes to the $NODE_VERISON environment variable well after the nvm-exec.sh script is created and uploaded to them. So yeah that wouldn't work, at least not for me 😝

The only thing I'm now thinking is; wouldn't it be better to add a nvm:install task, and let that be idempotent, instead of having side effect when executing a node binary?

So like an nvm:install task that I could choose to invoke somewhere in my deploy, and then the wrapper script just runs nvm use before all of my commands?

While I think that this suggestion would probably be more elegant in some respects, nvm install is basically "Install if not installed already, then use." I think it makes sense to leverage the implicit behavior here since we basically get to let nvm worry about version management entirely just by configuring a ruby symbol.

koenpunt · 2018-05-21T17:22:19Z

lib/capistrano/tasks/nvm.rake


      unless test(nvm_node_path.map {|p| "[ -d #{p} ]" }.join(" || "))
-        error "nvm: #{nvm_node} is not installed or not found in any of #{nvm_node_path.join(" ")}"
-        exit 1
+        if nvm_verb == 'use'


Here you can actually move the verb comparison before the test command, which then omits a remote invocation when the verb is not use.

koenpunt · 2018-05-21T17:25:46Z

lib/capistrano/tasks/nvm.rake

@@ -9,17 +9,21 @@ namespace :nvm do

      nvm_node_path = fetch(:nvm_node_path)
      nvm_node_path = [nvm_node_path] unless nvm_node_path.is_a?(Array)
+      nvm_verb = fetch(:nvm_verb).to_s


Can you add validation for the verb, since it can only be :use or :install right?
But then again, if there are only 2 options, maybe it would be better to make it a boolean property, e.g. :nvm_install, which when true causes the "verb" to be install instead of use

Can you add validation for the verb,

Absolutely.

since it can only be :use or :install right?

Effectively yes, but let's discuss your other point...

But then again, if there are only 2 options, maybe it would be better to make it a boolean property, e.g. :nvm_install, which when true causes the "verb" to be install instead of use

Hmm... I'd need to rewrite some things to kind of unify the behavior of this code. For example, when specifying use instead of install, the node version availability check is path based, so the version string has to be exact and must prepend the v, but it really should be running nvm version on the destination machines instead.

I also personally think that :nvm_install behavior should default to true as nvm is just a lot more useful that way, at least from an automation standpoint. That's technically a major version change so I guess if I really think that's the right call I can always submit another PR.

...I've got a good plan for how to handle this, but I suppose I needed to "think out loud" to get the ideas right. LMK if anything doesn't seem right to you, otherwise I'll work on this sometime in the coming days and get that done.

koenpunt · 2022-10-30T16:24:20Z

Coming back at this PR once more; I think the verb nomenclature is too ambiguous, so I'd like to suggest a nvm_action option defaulting to "use", or an nvm_install action defaulting to false instead.

Add support for nvm_verb, update readme

bab078a

RulerOf mentioned this pull request Mar 12, 2018

Pull back to koenpunt add rubocop #28

Closed

koenpunt requested changes May 20, 2018

View reviewed changes

RulerOf added 2 commits May 21, 2018 12:00

Documentation updates for consistency at request of maintainer

7418178

Bump up to next minor version number

cf4babe

Backward-compatible changes were made

koenpunt reviewed May 21, 2018

View reviewed changes

tiagotex mentioned this pull request Aug 9, 2021

Still active? #34

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for nvm_verb, update readme #27

Add support for nvm_verb, update readme #27

RulerOf commented Mar 12, 2018 •

edited

Loading

RulerOf commented Apr 16, 2018

koenpunt left a comment

koenpunt May 20, 2018

koenpunt May 20, 2018

RulerOf May 21, 2018

koenpunt May 20, 2018

RulerOf May 21, 2018

koenpunt May 20, 2018

koenpunt May 20, 2018

RulerOf May 21, 2018

RulerOf May 21, 2018

koenpunt May 21, 2018

RulerOf May 21, 2018 •

edited

Loading

koenpunt May 21, 2018

koenpunt May 21, 2018

RulerOf May 21, 2018 •

edited

Loading

koenpunt May 21, 2018

RulerOf May 21, 2018 •

edited

Loading

koenpunt May 21, 2018

koenpunt May 21, 2018 •

edited

Loading

RulerOf May 21, 2018

koenpunt commented Oct 30, 2022

Add support for nvm_verb, update readme #27

Are you sure you want to change the base?

Add support for nvm_verb, update readme #27

Conversation

RulerOf commented Mar 12, 2018 • edited Loading

RulerOf commented Apr 16, 2018

koenpunt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RulerOf May 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RulerOf May 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RulerOf May 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koenpunt May 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koenpunt commented Oct 30, 2022

RulerOf commented Mar 12, 2018 •

edited

Loading

RulerOf May 21, 2018 •

edited

Loading

RulerOf May 21, 2018 •

edited

Loading

RulerOf May 21, 2018 •

edited

Loading

koenpunt May 21, 2018 •

edited

Loading