-
-
Notifications
You must be signed in to change notification settings - Fork 366
fix(generator): make output permission error clear and user-friendly #1844
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
base: master
Are you sure you want to change the base?
fix(generator): make output permission error clear and user-friendly #1844
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.
Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.
What reviewer looks at during PR reviewThe following are ideal points maintainers look for during review. Reviewing these points yourself beforehand can help streamline the review process and reduce time to merge.
|
📝 WalkthroughWalkthroughWraps directory creation in the generator with a try/catch to intercept filesystem permission errors (EACCES) and throw a user-friendly permission-denied error; other errors are rethrown. Subsequent directory validation is moved outside the try/catch, preserving forceWrite behavior. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/generator/lib/generator.js`:
- Around line 248-260: In setupFSOutput(), the catch currently only treats
err.code === 'EACCES' as a friendly permission error; update the conditional to
also handle 'EPERM' and 'EROFS' so permission and read-only filesystem errors
are presented with the same user-friendly message for this.targetDir—i.e., check
if err.code is one of ['EACCES','EPERM','EROFS'] and throw the same descriptive
Error, otherwise rethrow the original err.
🧹 Nitpick comments (1)
apps/generator/lib/generator.js (1)
248-266: Add a regression test and align the JSDoc for the new error path.Since this change improves user-facing messaging, a focused test that mocks
fs.extra.mkdirpSyncto throwEACCES(and asserts the new message) will guard against regressions. Also update the JSDoc abovesetupFSOutput()to mention permission-denied errors in its@throwsdescription.Based on learnings, ensure any new tests are placed under
apps/generator/test/and follow existing patterns.
|
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.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/generator/lib/generator.js (1)
237-247: UpdatesetupFSOutputJSDoc to mention permission-denied errors.The
@throwsdescription only mentions verification failures, but the method now also throws when permissions prevent writing. Please update the doc to reflect that new error path.As per coding guidelines, ensure error conditions are documented.📝 Suggested doc update
- * `@throws` {Error} If verification of the target directory fails and forceWrite is not enabled. + * `@throws` {Error} If the output directory is not writable (EACCES/EPERM/EROFS), or if verification fails when forceWrite is not enabled.
🧹 Nitpick comments (2)
apps/generator/lib/generator.js (2)
248-260: Consider verifying write access for existing output directories.If
xfs.mkdirpSyncbehaves like standardmkdirpand doesn’t throw for an existing-but-non‑writable directory, the friendly message won’t trigger and later writes can still surface raw permission errors. Adding an explicit write-access check in the same try/catch would cover that case.🔧 Suggested update
try { xfs.mkdirpSync(this.targetDir); + fs.accessSync(this.targetDir, fs.constants.W_OK); } catch (err) { if (['EACCES', 'EPERM', 'EROFS'].includes(err.code)) { throw new Error( `Permission denied: cannot write to output directory "${this.targetDir}". ` + `Please choose a directory you have write access to, or adjust the permissions for the target directory.` ); } throw err; }
254-257: Consider updating user-facing documentation to match the new error copy.Since the CLI message changed, it may be worth updating Generator/CLI docs so users see consistent guidance.
Based on learnings, align docs with user-facing changes.



Description
EACCESpermission errors with a clear, user-friendly message.Related issue(s)
fixes #1842
Before
After
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.