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

remove nvm installation for SDK automation #30607

Merged

Conversation

kazrael2119
Copy link
Contributor

@kazrael2119 kazrael2119 commented Aug 1, 2024

@maorleger
Copy link
Member

maorleger commented Aug 1, 2024

Rather than removing the installation steps, could we instead check and skip if nvm is already installed?

Edit: I am not that worried about automation_init scripts so feel free to ignore

@wanlwanl
Copy link
Member

wanlwanl commented Aug 2, 2024

@raych1 for awareness

@raych1
Copy link
Member

raych1 commented Aug 2, 2024

@kazrael2119
The code to call NVM to set node version can also be removed since the pipeline agent doesn't have NVM installed.
Both the init script and the generate script are currently only used in the spec PR pipeline which has already installed the Node. We can ignore the NVM/Node installation in these script.

@jeremymeng
Copy link
Contributor

The automation probably don't need to use nvm to switch nodejs versions? Looking at the history nvm was used to install node v18, likely when 18 is not the default installed version.

@jeremymeng
Copy link
Contributor

/cc current code owners @ckairen @mikeharder please let me know if we should have better owners for these files.

@raych1
Copy link
Member

raych1 commented Aug 8, 2024

The automation probably don't need to use nvm to switch nodejs versions? Looking at the history nvm was used to install node v18, likely when 18 is not the default installed version.

@jeremymeng
This PR is to revert the change added previously to install NVM to install node18 when the spec PR pipeline didn't support node18. The spec PR has already supported the node 18 and it will upgrade the node version per the lifetime. So, it doesn't need the JS language automation script to install node separately.

@wanlwanl wanlwanl requested review from xirzec and a team as code owners August 13, 2024 06:02
@wanlwanl wanlwanl force-pushed the remove-nvm-installation-for-automation branch from 2e7e1f1 to 5a0df1c Compare August 13, 2024 07:19
@wanlwanl wanlwanl enabled auto-merge (squash) August 13, 2024 07:22
@wanlwanl
Copy link
Member

wanlwanl commented Aug 13, 2024

@jeremymeng @raych1 I found the checkenforcer is always in pending status. but when I click button, the status is already success. could you help merge the pr?

@jeremymeng
Copy link
Contributor

/check-enforcer override

@jeremymeng jeremymeng merged commit 1c87b11 into Azure:main Aug 13, 2024
6 checks passed
@kazrael2119 kazrael2119 deleted the remove-nvm-installation-for-automation branch August 14, 2024 02:03
@maorleger maorleger mentioned this pull request Aug 14, 2024
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.

[SDK Automation] JS automation script doesn't need to install 'node' repeatedly
5 participants