chore: Move appsmithctl to RTS #37531
Conversation
WalkthroughThe pull request introduces several modifications across various files, primarily focusing on the Dockerfile, build configurations, and package management. Key changes include the simplification of the Dockerfile by removing unnecessary paths and commands, the addition of new entry points in the build process, updates to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
💤 Files with no reviewable changes (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (2)CODEOWNERS (2)
The new ownership entry for
The ownership assignment for cypress tests to the QA team members is appropriate. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
the
skip-changelog
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
app/client/packages/rts/tsconfig.json (1)
Line range hint
3-3: Remove duplicate "lib" configurationThe "lib" field is defined twice - once in compilerOptions and once at the root level. The root level entry appears to be outdated (es2015).
Remove the duplicate entry:
{ "compilerOptions": { "target": "es6", "lib": ["ES2021"], ... }, "include": ["src/**/*", "src/**/*.json"], "exclude": ["jest.config.js", "src/test", "index.js", "build.js", "src/ctl"], - "lib": ["es2015"] }Also applies to: 25-25
app/client/packages/rts/package.json (1)
26-26: Consider updating dotenv to a newer versionThe current version (10.0.0) is quite old. Consider upgrading to the latest stable version (16.x) for improved features and security fixes.
app/client/packages/rts/build.js (1)
Line range hint
47-58: Consider separating build configurations for different entry points.Since
src/ctl/index.jsis a JavaScript file whilesrc/server.tsis TypeScript, consider creating separate build configurations to handle their specific needs more effectively.Example refactor:
- entryPoints: ["src/server.ts", "src/ctl/index.js"], - bundle: true, - sourcemap: true, - platform: "node", - external: [...externalWorkflowPackages, "dtrace-provider"], - loader: { - ".ts": "ts", - }, - tsconfig: "tsconfig.json", - outdir: "dist/bundle", - target: "node" + process.versions.node, - minify: true, - keepNames: true, + // Common options + const commonOptions = { + bundle: true, + sourcemap: true, + platform: "node", + external: [...externalWorkflowPackages, "dtrace-provider"], + outdir: "dist/bundle", + target: "node" + process.versions.node, + minify: true, + keepNames: true, + }; + + // Server build + await esbuild.build({ + ...commonOptions, + entryPoints: ["src/server.ts"], + loader: { ".ts": "ts" }, + tsconfig: "tsconfig.json", + }); + + // CTL build + await esbuild.build({ + ...commonOptions, + entryPoints: ["src/ctl/index.js"], + });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
app/client/yarn.lockis excluded by!**/yarn.lock,!**/*.lockdeploy/docker/fs/opt/appsmith/utils/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
Dockerfile(1 hunks)app/client/packages/rts/.eslintrc.json(1 hunks)app/client/packages/rts/build.js(1 hunks)app/client/packages/rts/package.json(1 hunks)app/client/packages/rts/src/ctl/README.md(1 hunks)app/client/packages/rts/tsconfig.json(1 hunks)deploy/docker/fs/opt/appsmith/utils/package.json(0 hunks)deploy/docker/fs/opt/bin/appsmithctl(1 hunks)deploy/docker/fs/opt/bin/ctl(1 hunks)scripts/local_testing.sh(1 hunks)
💤 Files with no reviewable changes (1)
- deploy/docker/fs/opt/appsmith/utils/package.json
✅ Files skipped from review due to trivial changes (4)
- app/client/packages/rts/.eslintrc.json
- app/client/packages/rts/src/ctl/README.md
- deploy/docker/fs/opt/bin/appsmithctl
- deploy/docker/fs/opt/bin/ctl
🔇 Additional comments (5)
app/client/packages/rts/tsconfig.json (1)
24-24: LGTM on excluding ctl directory
The exclusion aligns with the PR objective of postponing TypeScript migration for the ctl folder.
Let's verify no TypeScript files exist in the ctl directory:
✅ Verification successful
Confirmed: Excluding ctl directory is appropriate
The src/ctl directory contains only JavaScript files (.js and .mjs), confirming that excluding it from TypeScript compilation is the correct approach until migration is planned.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any TypeScript files in src/ctl
fd -e ts -e tsx . app/client/packages/rts/src/ctl
Length of output: 51
Script:
#!/bin/bash
# First verify if the ctl directory exists
fd ctl app/client/packages/rts/src
# Check all files in ctl directory if it exists
find app/client/packages/rts/src/ctl -type f 2>/dev/null || echo "Directory not found"
# Double check with ripgrep for any potential TypeScript imports or references
rg -g '!*.d.ts' -g '!*.test.ts' -l "from ['\"]\./ctl/" app/client/packages/rts/src
Length of output: 926
app/client/packages/rts/package.json (1)
26-26: Verify security advisories for new dependencies
Let's check for any known vulnerabilities in the newly added dependencies.
Also applies to: 31-33
✅ Verification successful
Let me check the exact versions of these dependencies to provide accurate security advice.
Security advisory check reveals vulnerabilities requiring attention
Based on the security scan and package versions:
dotenv@10.0.0- No known vulnerabilitiesmongodb@5.8.0- No vulnerabilities (above the vulnerable range < 5.8.0)nodemailer@6.9.9- Using latest patched versionreadline-sync@1.4.10- No known vulnerabilities
All dependencies are either using patched versions or have no known vulnerabilities. The code can proceed as is.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for security advisories in new dependencies
echo "Checking security advisories for new dependencies..."
for pkg in "dotenv" "mongodb" "nodemailer" "readline-sync"; do
echo "Checking $pkg..."
gh api graphql -f query='{
securityVulnerabilities(first: 5, ecosystem: NPM, package: "'$pkg'") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
done
Length of output: 3616
Script:
#!/bin/bash
# Check package.json for exact versions
cat app/client/packages/rts/package.json | grep -A5 '"dependencies":'
Length of output: 343
Script:
#!/bin/bash
# Let's get the full package.json content to see all dependencies
cat app/client/packages/rts/package.json
Length of output: 1474
Dockerfile (2)
Line range hint 32-52: LGTM: Secure file permission setup
The permission setup follows security best practices:
- Disabling setuid/setgid bits
- Explicit executable permissions for scripts
- Write permissions limited to necessary directories
32-32: Verify PATH modification impact
The removal of /opt/appsmith/utils/node_modules/.bin from PATH aligns with moving appsmithctl to RTS, but let's verify no other scripts depend on utils binaries.
✅ Verification successful
PATH modification is safe to proceed
The removal of /opt/appsmith/utils/node_modules/.bin from PATH appears safe as no references to utils binaries were found in the codebase. This aligns with the PR objective of moving appsmithctl to RTS.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any references to executables that might have been in utils/node_modules/.bin
rg -g '!{node_modules,dist,build}' --type sh '/opt/appsmith/utils/node_modules/.bin'
# Check for any package.json scripts that might reference utils binaries
fd package.json --type f --exec jq -r '.scripts | values[]' {} \; | rg '/opt/appsmith/utils/node_modules'
Length of output: 193
Script:
#!/bin/bash
# Let's try a different approach to find any dependencies on utils binaries
# Search for any shell scripts that might reference utils directory
rg -g '!{node_modules,dist,build}' --type sh 'utils/node_modules' -l
# Search for any references to utils in package.json files
fd package.json --type f --exec grep -l "utils/node_modules" {} \;
# Check if there are any binaries in utils/node_modules/.bin that we need to be aware of
fd -t f . -p '**/utils/node_modules/.bin/*'
# Look for any references to utils binaries in shell scripts or configuration files
rg -t sh -t yaml -t json 'utils/node_modules/.bin' -l
Length of output: 354
app/client/packages/rts/build.js (1)
47-47: Verify the existence of the new entry point file.
The build configuration now includes src/ctl/index.js as an entry point.
✅ Verification successful
Entry point file exists and is properly configured
The new entry point src/ctl/index.js exists in the correct location and the build configuration is properly set up to handle it with the existing loader configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of the new entry point file
# and check its contents for potential issues
# Check if the file exists
fd --type f "index.js" "src/ctl"
# Check the file contents if it exists
rg -A 5 "exports|module.exports" "src/ctl/index.js" || echo "No exports found"
Length of output: 277
Script:
#!/bin/bash
# Let's verify if the file exists in the repository root or app/client/packages/rts directory
fd --type f "index.js" "app/client/packages/rts"
# Check the build.js file to understand the context better
cat "app/client/packages/rts/build.js"
Length of output: 2560
CODEOWNERS
Outdated
There was a problem hiding this comment.
Errors in this file fixed in #37553.
|
@sharat87 Can you help me understand he rationale behind this change? Why are we clubbing rts with Appsmithctl? Won't this lead to confusion around what rts is responsible for? I'm asking because rts is a service while ctl is a command line utility. |
|
@mohanarpit, thanks for asking. I should've clarified that in the description. Let me do that here. You're right. RTS is an API service, and If Another way I think about this is, some of our backend logic, like AST parsing, SCIM support, are implemented in RTS, almost solely because they're Javascript. Well, Overall, decision to have Does that sound okay? |
Move the `appsmithctl` code to RTS. RTS' own build system will build `appsmithctl` as well. We're adding two command scripts, `ctl` and `appsmithctl` to `/opt/bin`, which will be the entrypoints for this. The `appsmithctl` is now just an alias to the much shorter and non-redundancy-inducing `ctl`. We aren't migrating to TypeScript in this PR so we're ignoring the new `ctl` folder in both `tsconfig.json` and `.eslintrc`. That's temporary, the next PR will fix that. ## Automation /test sanity ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/11930931528> > Commit: 90b5f97 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=11930931528&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Sanity` > Spec: > <hr>Wed, 20 Nov 2024 10:36:02 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes - **New Features** - Introduced the `appsmithctl` command for easier command execution. - Updated build process to include additional entry points. - **Bug Fixes** - Streamlined Docker build process, enhancing efficiency and reducing complexity. - **Documentation** - Added a new section in the README for `appsmithctl` command description. - **Chores** - Updated dependencies in `package.json`. - Removed obsolete files and workflows to simplify project structure. - **Style** - Added a new ESLint configuration for specific project needs. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Move the
appsmithctlcode to RTS.RTS' own build system will build
appsmithctlas well. We're adding two command scripts,ctlandappsmithctlto/opt/bin, which will be the entrypoints for this. Theappsmithctlis now just an alias to the much shorter and non-redundancy-inducingctl.We aren't migrating to TypeScript in this PR so we're ignoring the new
ctlfolder in bothtsconfig.jsonand.eslintrc. That's temporary, the next PR will fix that.Automation
/test sanity
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11930931528
Commit: 90b5f97
Cypress dashboard.
Tags:
@tag.SanitySpec:
Wed, 20 Nov 2024 10:36:02 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Release Notes
New Features
appsmithctlcommand for easier command execution.Bug Fixes
Documentation
appsmithctlcommand description.Chores
package.json.Style