-
Notifications
You must be signed in to change notification settings - Fork 0
add 'require-aws-config' rule #134
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.
Pull Request Overview
This PR adds a new ESLint rule called 'require-aws-config' that enforces the use of @checkdigit/aws-config when working with AWS SDK v3 clients instead of instantiating them directly.
- Implements a new ESLint rule to detect direct AWS client instantiation
- Adds utility function to detect AWS SDK v3 usage in projects
- Integrates the rule into the existing ESLint configuration system
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/require-aws-config.ts | Main rule implementation that detects new *Client() patterns |
| src/require-aws-config.spec.ts | Test cases covering valid and invalid usage scenarios |
| src/is-aws-sdk-v3-used.ts | Utility to determine if AWS SDK v3 is used in the project |
| src/index.ts | Integration of the new rule into the plugin's rule registry |
| eslint.config.mjs | Configuration to enable AWS SDK v3 detection |
| docs/rules/require-aws-config.md | Documentation with usage examples |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
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.
Pull Request Overview
Copilot reviewed 11 out of 12 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
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.
Pull Request Overview
Copilot reviewed 11 out of 12 changed files in this pull request and generated no new comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/require-aws-config.ts
Outdated
| defaultOptions: [], | ||
| create(context) { | ||
| return { | ||
| ImportDeclaration(node) { |
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.
Should we have check for imports of checkdigit/aws and report as well?
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.
good point, will add the check.
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.
Pull Request Overview
Copilot reviewed 11 out of 12 changed files in this pull request and generated 4 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| import requireAssertMessage from './require-assert-message'; | ||
| import requireTsExtensionImportsExports from './require-ts-extension-imports-exports.ts'; | ||
|
|
||
| export { default as isAwsSdkV3Used } from './is-aws-sdk-v3-used.ts'; |
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.
Do we need to export?
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.
yes, it's needed for eslint-config to call asynchronously and put into settings as part of the rule configuration
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.
isAwsSdkV3Used directly imported from ./src/is-aws-sdk-v3-used.ts in the eslint.config.mjs
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.
removed since it's not applicable for rules themselves
jpolavar
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.
lftm
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.
Pull Request Overview
Copilot reviewed 11 out of 12 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
❌ PR review status - has 1 reviewer outstanding |
|
@le-cong I've re-enabled the CodeQL action but you might need to do another commit to pass the check |
|
Beta Published - Install Command: |
jpolavar
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.
lftm
Fixes #132