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

CLI: Fix nvm setup #241

Merged
merged 3 commits into from
Feb 23, 2024
Merged

CLI: Fix nvm setup #241

merged 3 commits into from
Feb 23, 2024

Conversation

jhnstn
Copy link
Member

@jhnstn jhnstn commented Jan 26, 2024

Fixes #240

This sets up the npm command to correctly use nvm.
It also provides a clear space to introduce other node managers.

Testing

Follow the Testing guide to set up the tool to use forked repos.

Prerequisite: nvm installed

Testing with nvm

  • Verify nvm is not loaded in bash_profile , e.g.
$ bash -l 
$ nvm  
bash: nvm: command not found
  • Make sure your default node in nvm is not v20
  • Try running go run main.go release prepare gb {Version}
  • Verify that the command reaches the "create PR" confirmation step

Testing without nvm

  • Make sure your default node in nvm is not v20
  • Try NVM_DIR= go run main.go release prepare gb {Version}
  • This should fail with unsupported engine error
  • Set the default node to v20 e.g. nvm alias default v20
  • Try NVM_DIR= go run main.go release prepare gb {Version} again
  • Verify that the command reaches the "create PR" confirmation step

@derekblank derekblank self-assigned this Jan 31, 2024
@jhnstn jhnstn enabled auto-merge (squash) February 7, 2024 21:29
Copy link
Member

@dcalhoun dcalhoun left a comment

Choose a reason for hiding this comment

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

I tested this while using asdf-vm without nvm installed. The script works as expected, utilizing the "system" Node.js.

Whenever my default Node.js is set to the desired version, the script uses it. I may explore how I might tweak my system bash configuration to expose my local, project-specific Node.js version controlled by asdf-vm to the script environment.

I'll let others provide feedback on the nvm support, but this looks ready to merge from. my perspective.

Copy link
Member

@dcalhoun dcalhoun left a comment

Choose a reason for hiding this comment

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

Optimistically approving nvm support even though I did not test it directly. Others can test and provide feedback in retrospect, if desired.

@jhnstn jhnstn merged commit b491fea into trunk Feb 23, 2024
2 checks passed
@jhnstn jhnstn deleted the fix/nvm-setup branch February 23, 2024 21:10
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.

[CLI] Fix logic to load nvm when calling npm commands
3 participants