feat: exit if docker buildx can't be found for self-hosted builds #1475
Conversation
WalkthroughThe changes introduced in this pull request enhance the deployment process within the Changes
Possibly related PRs
Warning Rate limit exceeded@ghostdevv has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 9 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
packages/cli-v3/src/commands/deploy.ts(node:30941) ESLintIgnoreWarning: The ".eslintignore" file is no longer supported. Switch to using the "ignores" property in "eslint.config.js": https://eslint.org/docs/latest/use/configure/migration-guide#ignoring-files Oops! Something went wrong! :( ESLint: 9.15.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
CodeRabbit Configuration File (
|
🦋 Changeset detectedLatest commit: 87acfe1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
.changeset/spicy-turkeys-hug.md (1)
5-5: Consider enhancing the commit message with more detailsWhile the message follows the conventional commit format, it could be more informative by including:
- The specific error message users will see
- The reason for this check (--progress flag requirement)
- The recommended solution for users
Consider expanding it to:
-feat: exit if docker buildx can't be found for self-hosted builds +feat: exit if docker buildx can't be found for self-hosted builds + +Adds a prerequisite check for docker buildx when using --self-hosted flag. +Exits with a clear error message if buildx is not installed, as it's +required for the --progress flag to work correctly.packages/cli-v3/src/commands/deploy.ts (2)
40-40: Consider using a more descriptive import name.The imported function name
xis not descriptive of its purpose. Consider renaming it to something more meaningful likeexecCommandorshellExec.-import { x } from "tinyexec"; +import { x as execCommand } from "tinyexec";
269-276: Enhance error message with installation instructions.The error handling is good, but the error message could be more helpful by including installation instructions for Docker buildx.
- throw new Error("Failed to find docker buildx, please install it."); + throw new Error( + "Failed to find docker buildx. Please install it by following the instructions at: " + + "https://docs.docker.com/build/buildx/install/" + );Also, consider including stdout/stderr in the debug log for better debugging:
- logger.debug(`"docker buildx version" failed (${result.exitCode}):`, result); + logger.debug( + `"docker buildx version" failed (${result.exitCode}):`, + { ...result, stdout: result.stdout, stderr: result.stderr } + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
.changeset/spicy-turkeys-hug.md(1 hunks)packages/cli-v3/package.json(1 hunks)packages/cli-v3/src/commands/deploy.ts(2 hunks)
🔇 Additional comments (4)
.changeset/spicy-turkeys-hug.md (1)
1-3: LGTM: Appropriate version bump for new feature
The minor version bump is correctly specified and follows semantic versioning principles for feature additions.
packages/cli-v3/package.json (2)
Line range hint 134-142: LGTM! Proper ESM module structure
The exports field is correctly configured for ESM modules, including proper type definitions. This aligns with modern Node.js practices.
121-121: Verify tinyexec upgrade compatibility
The upgrade from ^0.2.0 to ^0.3.1 is used for Docker buildx checks. Let's verify the changes between versions.
packages/cli-v3/src/commands/deploy.ts (1)
269-276: LGTM! Well-integrated buildx check.
The Docker buildx check is well-placed in the deployment flow:
- Fails fast before any resource-intensive operations
- Logically grouped with other build prerequisites
- Properly guarded by the
selfHostedoption
@trigger.dev/react-hooks
@trigger.dev/sdk
trigger.dev
@trigger.dev/build
@trigger.dev/core
commit: |
There was a problem hiding this comment.
Hey @ghostdevv - good spot! Thanks for this 🙏
The tinyexec upgrade should probably be fine but I'll give it a quick spin after merging.
I ran into an issue when using trigger that I docker would fail to build as it couldn't find the
--progressflag. This flag is only available when docker buildx is installed. This change adds in a check to see if docker buildx is installed, and if it's not it'll fail with a helpful message.When buildx is installed the
docker buildx versioncommand will return a version, if it's not then the command will fail.(
install-docker-buildxisn't a real command, I just wrapped the arch linuxyay -S docker-buildxfor the demo)We can then test the warning I added based on this
Additionally, I added a debug log for this. You can test that by running
trigger deploy --self-hosted --log-level debug, and you'll get something like this at the end of the log:Edit: added a better build message based on rabbit's suggestion
✅ Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Chores
tinyexecdependency to version^0.3.1.package.jsonto version3.2.0and refined module export structure.