fix: Refactor tf scanner to reference resources #245
Conversation
WalkthroughThe changes in this pull request involve updates to multiple Changes
Possibly related PRs
Suggested reviewers
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
|
![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 (5)
integrations/terraform-cloud-scanner/src/scanner.ts (1)
Line range hint
37-83: Improved error handling and functional programming approachThe refactoring from a traditional loop to
Promise.allSettledwithmapandfilteroperations provides better error handling and a more functional approach. The use ofisPresenttype guard ensures type safety.Consider adding error logging for failed promises:
const resources = await Promise.allSettled(resourcePromises).then( (results) => results - .map((result) => - result.status === "fulfilled" ? result.value : null, - ) + .map((result) => { + if (result.status === "rejected") { + logger.error("Failed to process workspace:", result.reason); + return null; + } + return result.value; + }) .filter(isPresent), );integrations/terraform-cloud-scanner/package.json (1)
Node.js version compatibility issue detected
The project has mixed Node.js version requirements:
- Root package.json requires
>=20.12.0- GitHub Actions workflow uses Node.js 22
- terraform-cloud-scanner has no explicit Node.js version requirement
Using
@types/nodefrom thenode22catalog in terraform-cloud-scanner might cause compatibility issues since the project's base requirement is Node.js 20. You should:
- Add
"node": ">=20.12.0"to terraform-cloud-scanner's package.json to align with the root package- Consider using
@types/nodefrom thenode20catalog instead🔗 Analysis chain
Line range hint
41-41: Verify Node.js version compatibilityThe package uses
@types/nodefrom thenode22catalog. Let's ensure this aligns with the project's Node.js version requirements.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check Node.js version requirements across the project # Check for explicit Node.js version requirements in package.json files rg -l '"node":' --type json # Check for .nvmrc or .node-version files fd -e nvmrc -e node-version # Check for Node.js version in GitHub workflow files fd -e yml -e yaml --exec rg -l 'node-version:' {} \;Length of output: 210
Script:
#!/bin/bash # Let's check the actual Node.js version requirements in the found files # Check package.json files echo "=== Root package.json ===" cat package.json | jq '.engines.node' echo -e "\n=== integrations/github-get-job-inputs/package.json ===" cat integrations/github-get-job-inputs/package.json | jq '.engines.node' echo -e "\n=== integrations/terraform-cloud-scanner/package.json ===" cat integrations/terraform-cloud-scanner/package.json | jq '.engines.node' # Check GitHub workflow Node.js version echo -e "\n=== GitHub Action Node.js version ===" cat tooling/github/setup/action.yml | grep -A 1 'node-version:'Length of output: 728
packages/job-dispatch/package.json (2)
Line range hint
29-31: Consider consolidating Redis clients.The package uses both
ioredisandredispackages. Consider standardizing on a single Redis client library to reduce dependency overhead and potential version conflicts.
Line range hint
29-42: Consider moving more dependencies to catalog references.For consistency in dependency management, consider moving other stable dependencies like
ioredis,lodash,ms,murmurhash,redis, andsemverto catalog references as well.apps/webservice/package.json (1)
Line range hint
16-85: Consider expanding catalog reference usage.Many dependencies could potentially use catalog references for consistent version management across the organization. Consider moving stable dependencies like
@hookform/resolvers,@icons-pack/react-simple-icons,@internationalized/date, etc., to catalog references.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
apps/jobs/package.json(1 hunks)apps/webservice/package.json(1 hunks)integrations/terraform-cloud-scanner/package.json(1 hunks)integrations/terraform-cloud-scanner/src/__tests__/scanner.test.ts(4 hunks)integrations/terraform-cloud-scanner/src/config.ts(1 hunks)integrations/terraform-cloud-scanner/src/scanner.ts(8 hunks)packages/job-dispatch/package.json(1 hunks)pnpm-workspace.yaml(1 hunks)
🔇 Additional comments (13)
integrations/terraform-cloud-scanner/src/config.ts (1)
13-15: Verify environment variable usage across the codebase
The environment variable has been renamed from CTRLPLANE_WORKSPACE_TARGET_NAME to CTRLPLANE_WORKSPACE_RESOURCE_NAME. While the change is consistent with the shift from "targets" to "resources" terminology, we should verify all references are updated.
✅ Verification successful
Environment variable renaming is consistent across the codebase
The search results confirm that all references to the environment variable have been updated to use the new name CTRLPLANE_WORKSPACE_RESOURCE_NAME. No instances of the old name CTRLPLANE_WORKSPACE_TARGET_NAME were found in the TypeScript files. The changes appear in:
- Configuration schema (config.ts)
- Scanner implementation (scanner.ts)
- Test files (scanner.test.ts)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the old environment variable name
rg "CTRLPLANE_WORKSPACE_TARGET_NAME" --type ts
# Search for references to the new environment variable name
rg "CTRLPLANE_WORKSPACE_RESOURCE_NAME" --type ts
Length of output: 564
integrations/terraform-cloud-scanner/src/__tests__/scanner.test.ts (3)
28-28: LGTM: Environment variable mock updated correctly
The mock configuration has been properly updated to use the new environment variable name.
122-129: LGTM: API endpoint and request structure updated consistently
The changes to the API endpoint and request body structure are consistent with the terminology change from "targets" to "resources".
161-163: LGTM: Log assertion updated appropriately
The log assertion has been updated to match the new terminology.
integrations/terraform-cloud-scanner/src/scanner.ts (4)
14-14: LGTM: Environment variable reference updated
The template now correctly uses the new environment variable name.
21-23: LGTM: Clean ResourceProvider initialization
Good use of object destructuring and the new ResourceProvider class.
104-107: LGTM: Improved variable processing
The variable processing has been simplified with a cleaner map operation.
Line range hint 129-141: LGTM: Consistent function style
Good refactoring of processVcsRepo and buildWorkspaceLink to use arrow function syntax, maintaining consistency with other utility functions.
Also applies to: 148-152
pnpm-workspace.yaml (1)
25-25: Verify the version constraint for ts-is-present
The addition of ts-is-present with ^1.2.2 to the catalog is part of centralizing dependency management. Let's verify this version choice.
✅ Verification successful
Let me verify how this version is being used in the codebase to ensure it's appropriate.
Version ^1.2.2 for ts-is-present is appropriate and secure
The version choice is verified to be suitable because:
- 1.2.2 is the latest stable version (released Sept 2021)
- No security vulnerabilities were found
- The package is being used consistently across multiple components, with most referencing the catalog entry
- One existing direct reference in packages/api uses the same ^1.2.2 constraint
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the latest versions and any known issues for ts-is-present
# Check NPM for latest versions and release dates
curl -s https://registry.npmjs.org/ts-is-present | jq -r '.time | to_entries | .[] | "\(.key) \(.value)"' | grep -v -E "created|modified"
# Check for any security advisories
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: NPM, package: "ts-is-present") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
Length of output: 827
Script:
#!/bin/bash
# Check how ts-is-present is being used across the codebase
rg "ts-is-present" --type json -A 2 -B 2
# Find all package.json files referencing the catalog
rg '"catalog:ts-is-present"' --type json
Length of output: 1509
apps/jobs/package.json (1)
22-22: LGTM! Consistent use of catalog references
The changes properly reference the centralized versions from the workspace catalog, maintaining consistency across the project.
Also applies to: 28-31
integrations/terraform-cloud-scanner/package.json (1)
34-34: LGTM! Consistent use of catalog references
The changes properly reference the centralized versions from the workspace catalog, maintaining consistency across the project.
Also applies to: 42-45
packages/job-dispatch/package.json (1)
41-41: LGTM on catalog reference update.
The change to use catalog reference for ts-is-present aligns with the PR objective of standardizing dependency management.
apps/webservice/package.json (1)
86-86: LGTM on ts-is-present catalog reference.
The change to use catalog reference for ts-is-present aligns with the PR objective and maintains consistency across packages.
| "ts-is-present": "catalog:", | ||
| "uuid": "^10.0.0", | ||
| "zod": "catalog:" |
There was a problem hiding this comment.
Fix inconsistency in uuid dependency version.
The uuid dependency still shows ^10.0.0 while the AI summary indicates it should be changed to catalog:. Please update this for consistency.
- "uuid": "^10.0.0",
+ "uuid": "catalog:",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "ts-is-present": "catalog:", | |
| "uuid": "^10.0.0", | |
| "zod": "catalog:" | |
| "ts-is-present": "catalog:", | |
| "uuid": "catalog:", | |
| "zod": "catalog:" |
Summary by CodeRabbit
New Features
Bug Fixes
CTRLPLANE_WORKSPACE_TARGET_NAMEtoCTRLPLANE_WORKSPACE_RESOURCE_NAMEfor consistency.Refactor
scanfunction for improved readability and efficiency, including concurrent processing of workspaces.Tests
Chores
package.jsonfiles across multiple projects to utilize catalog references for dependencies.