-
Couldn't load subscription status.
- Fork 33
adapted sdk generation to unix environment #1648
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
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.
Hey @nitrosx - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider using Docker volume user mapping (--user flag) instead of requiring sudo privileges. This would be a more secure approach than running the entire script with root permissions.
- Please update the test status in the PR description to indicate whether the changes have been tested on Unix environments.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| rm -rf @scicatproject/scicat-sdk-ts | ||
|
|
||
| echo -e "\nGenerating the new sdk..." | ||
| docker run \ |
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.
issue: Add error handling for Docker operations
Add checks to verify Docker is running and host.docker.internal is accessible before attempting operations. Handle potential errors explicitly.
| --additional-properties=ngVersion=16.2.12,npmName=@scicatproject/scicat-sdk-ts,supportsES6=true,npmVersion=10.8.2,withInterfaces=true | ||
|
|
||
| REMOVE_NPM_LINK=0 | ||
| if ! command -v npm 2>&1 1>/dev/null |
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.
issue (complexity): Consider using direct binary path variables instead of creating symlinks in system directories
The current approach of creating/removing symlinks in /usr/local/bin adds unnecessary complexity and risk. Instead, detect and use the correct binary paths directly:
# At the start of the script
if ! command -v npm >/dev/null 2>&1; then
if [[ -n "${NVM_BIN}" ]]; then
NPM_CMD="${NVM_BIN}/npm"
NODE_CMD="${NVM_BIN}/node"
if [[ ! -x "$NPM_CMD" ]] || [[ ! -x "$NODE_CMD" ]]; then
echo "Required binaries not found or not executable in NVM_BIN" >&2
exit 1
fi
else
echo "No npm found and NVM_BIN not set" >&2
exit 1
fi
else
NPM_CMD="npm"
NODE_CMD="node"
fi
# Then use these variables instead of global npm/node commands
"$NPM_CMD" install
"$NPM_CMD" run buildThis approach:
- Removes the need for symlink creation/cleanup
- Fails fast if requirements aren't met
- Uses full paths directly when needed
- Maintains the same functionality more safely
| } else { | ||
| console.log("Your environment is a linux/unix"); | ||
| console.log("Please run the following command on your terminal:"); | ||
| console.log("> sudo -E ./scripts/generate-nestjs-sdk.bash"); |
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.
@nitrosx I am thinking instead of asking people to run the command, can't we just run the command from here directly. Something like:
execSync( "sudo -E ./scripts/generate-nestjs-sdk.bash", { encoding: "utf-8" }, );
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.
Unfortunately it does not work in the current form of the bash script.
It does not pass the correct user and environment in the shell.
If we can find a way to do it from the javascript script, I fine with it
Description
This PR introduces bash script to generate locally the SDK on linux/unix environments.
Motivation
The javascript script to generate locally the SDK was failing due to permission issues as the docker container generate the sdk as the root user, while the script is mostly run as non privileged user
Fixes:
Please provide a list of the fixes implemented in this PR
Changes:
Please provide a list of the changes implemented by this PR
Tests included
Documentation
Summary by Sourcery
Adapt SDK generation process to support Unix environments by adding a bash script and updating the existing JavaScript script to handle OS-specific execution.
New Features:
Enhancements: