-
Notifications
You must be signed in to change notification settings - Fork 260
fix: [WIN-NPM] back-compat mitigation to PATH issue #1867
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
Conversation
vakalapa
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
npm/examples/windows/azure-npm.yaml
Outdated
| value: .\\etc\\azure-npm\\azure-npm.json | ||
| - name: PATH | ||
| value: '%CONTAINER_SANDBOX_MOUNT_POINT%\Windows\System32\WindowsPowershell\v1.0\' | ||
| value: '%CONTAINER_SANDBOX_MOUNT_POINT%\Windows\System32\WindowsPowershell\v1.0\;C:\Windows\System32\' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if overriding the PATH value here is the right choice, we should append this new PATH to the existing PATH. guess doing it in Dockerfiles is the right way ? or taking in this variable as NEWPATH and appending this NEWPATH to PATH in the Dockerfiles
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
appending now, although there might be a better fix that doesn't modify PATH. Looks like there was a regression in the NPM build some time after 02/06
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
appending with %PATH% doesn't actually append anything
| fieldPath: spec.nodeName | ||
| - name: NPM_CONFIG | ||
| value: .\\etc\\azure-npm\\azure-npm.json | ||
| - name: PATH |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove our first attempt at mitigation, which is not back-compat and is incomplete (can't use vfpctrl and other tools used by HNS to collect logs)
Reason for Change:
Resolve PATH issue correctly. Removing
powershell.exeis back-compat with NPM v1.3.34 (in-prod).Issue Fixed:
Requirements:
Notes:
Don't want to override PATH variable.
Error