fix get job inputs #196
fix get job inputs #196
Conversation
WalkthroughThis pull request introduces several changes across multiple files. The 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: 11
🧹 Outside diff range and nitpick comments (6)
github/get-job-inputs/index.js (2)
27997-28008: Add request timeout configurationThe API client creation lacks timeout configuration which could lead to hanging requests in case of network issues.
Consider adding timeout configuration:
function dist_createClient(options) { return createClient({ baseUrl: options.baseUrl ?? "https://app.ctrlplane.com", ...options, + timeout: options.timeout ?? 30000, // 30 second default timeout fetch: (input) => { const url = new URL(input.url); url.pathname = `/api${url.pathname}`; - return fetch(new Request(url.toString(), input)); + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), options.timeout ?? 30000); + return fetch(new Request(url.toString(), { ...input, signal: controller.signal })) + .finally(() => clearTimeout(timeoutId)); }, headers: { "x-api-key": options?.apiKey } }); }
28076-28096: Add retry mechanism for job operationsThe Job class methods lack retry mechanisms for transient failures, which could affect reliability.
Consider implementing a retry mechanism for critical operations:
+const MAX_RETRIES = 3; +const RETRY_DELAY = 1000; + +async function withRetry(operation) { + let lastError; + for (let i = 0; i < MAX_RETRIES; i++) { + try { + return await operation(); + } catch (error) { + lastError = error; + if (i < MAX_RETRIES - 1) { + await new Promise(resolve => setTimeout(resolve, RETRY_DELAY * Math.pow(2, i))); + } + } + } + throw lastError; +} + class Job { constructor(job, client) { this.job = job; this.client = client; } acknowledge() { - return this.client.POST("/v1/jobs/{jobId}/acknowledge", { + return withRetry(() => this.client.POST("/v1/jobs/{jobId}/acknowledge", { params: { path: { jobId: this.job.id } } - }); + })); } // Apply similar pattern to other methods }integrations/github-get-job-inputs/src/index.ts (3)
38-38: Redundant Type Annotation forjobIdThe explicit type annotation
: stringforjobIdmay be unnecessary sincecore.getInputreturns a string when{ required: true }is specified. You can simplify the code by removing the redundant type annotation.Apply this diff:
- const jobId: string = core.getInput("job_id", { required: true }); + const jobId = core.getInput("job_id", { required: true });
46-46: Use Strict Equality Comparison forundefinedUse the strict equality operator
===when checking ifdataisundefinedto avoid unexpected type coercion.Apply this diff:
- if (data == undefined) { + if (data === undefined) {
47-47: Improve Error Message for Undefined DataLogging
${data}whendataisundefinedwill output'undefined', which may not be helpful. Consider providing a more informative error message.Apply this diff:
- core.error(`Invalid Job data ${data}`); + core.error(`No job data returned for job ID: ${jobId}`);apps/webservice/src/app/api/v1/jobs/[jobId]/route.ts (1)
128-134: Restructure the response to avoid property overwritesBy spreading
...je.jobfollowed by...je, properties injemay overwrite those inje.job, leading to potential data inconsistencies. Consider restructuring the response to prevent this.Apply this diff to clarify the response structure:
return NextResponse.json({ - ...je.job, - ...je, + job: je.job, + runbook: je.runbook, + environment: je.environment, + deployment: je.deployment, + release: je.release, variables, target: targetWithMetadata, approval, });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
apps/webservice/src/app/api/openapi/page.tsx(1 hunks)apps/webservice/src/app/api/v1/jobs/[jobId]/route.ts(2 hunks)github/get-job-inputs/index.js(6 hunks)integrations/github-get-job-inputs/src/index.ts(3 hunks)integrations/github-get-job-inputs/src/sdk.ts(1 hunks)
🔥 Files not summarized due to errors (1)
- github/get-job-inputs/index.js: Error: Server error: no LLM provider could handle the message
🧰 Additional context used
🪛 Gitleaks
github/get-job-inputs/index.js
28120-28120: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (8)
integrations/github-get-job-inputs/src/sdk.ts (3)
1-3: LGTM! Clean and proper imports.
The imports are well-structured and follow TypeScript best practices.
5-8: Consider security implications of API key handling.
While GitHub Actions securely handles secrets, consider:
- Adding runtime checks for API key format
- Logging sanitization in case the API key is accidentally logged
Let's verify the action's security configuration:
#!/bin/bash
# Description: Check if the action.yml properly marks api_key as secret
# Test: Verify action.yml contains proper input configuration
rg -A 5 "api_key:" action.yml5-8: Verify error handling in dependent files.
Since this is a new SDK file, ensure that consumers properly handle potential errors from the client creation.
Let's check the error handling in dependent files:
✅ Verification successful
Let's gather more specific information about how the API client is used in the files that import it.
Based on the search results, I can see that error handling is already implemented in the dependent files that use the SDK client. Specifically:
- The GitHub job inputs integration has error handling:
.catch((error) => core.setFailed(`Action failed: ${error.message}`));- The Kubernetes job agent has comprehensive error handling:
catch (error: any) {
logger.error(`Error processing job ${job.id}`, {
error: error.message,
stack: error.stack,
});
}- The Terraform Cloud scanner has error handling:
catch (error) {
logger.error("Failed to register targets", {
error: error.message,
stack: error.stack,
});
}The SDK client is properly handled by consumers, with appropriate error logging and fallback behaviors.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how the api client is used in other files
# Test: Find files importing the SDK
rg -l "from ['\"].*sdk['\"]" --type ts
# Test: Check for try-catch blocks around api usage
ast-grep --pattern 'try {
$$$
api.$$$
$$$
} catch'
Length of output: 437
Script:
#!/bin/bash
# Check the actual usage of the api client in the importing files
rg "api\." -A 3 --type ts
# Also check for any existing error handling patterns
rg "catch" -B 3 --type ts
Length of output: 185396
apps/webservice/src/app/api/openapi/page.tsx (1)
9-9: Verify OpenAPI specification file existence.
Let's ensure the JSON specification file exists and is properly generated.
✅ Verification successful
OpenAPI specification file exists and appears valid ✅
The OpenAPI specification file (openapi.v1.json) exists at the repository root and contains valid OpenAPI 3.0.3 structure. The file appears to be properly maintained with multiple corresponding OpenAPI route definitions in the apps/webservice/src/app/api/v1/ directory.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify OpenAPI specification file existence and format
# Test 1: Check if the JSON file exists
echo "Checking for OpenAPI specification file..."
fd -t f "openapi.v1.json"
# Test 2: Validate if there's a corresponding build/generation script
echo "Checking for OpenAPI build scripts..."
rg -l "openapi.*\.(yaml|yml)" -g "package.json"
Length of output: 269
Script:
#!/bin/bash
# Let's verify the content and structure of the OpenAPI specification file
# Check if the file is a valid JSON and get its basic structure
echo "Checking OpenAPI specification file content..."
head -n 20 openapi.v1.json
# Look for any OpenAPI generation scripts in the codebase
echo "Searching for OpenAPI related scripts..."
rg -l "openapi" -g "*.{js,ts,json}" -g "!*.d.ts"
Length of output: 1950
github/get-job-inputs/index.js (1)
28098-28108: LGTM: Efficient unique array implementation
The uniqBy implementation is efficient using a Map for O(n) complexity.
integrations/github-get-job-inputs/src/index.ts (2)
59-59: Add Null Checks When Destructuring data Properties
When destructuring properties from data, ensure that data contains all the expected properties to prevent runtime errors.
Consider adding default values or null checks when destructuring:
- } = data;
+ } = data || {};42-44: Verify API Call Parameters and Endpoint
Ensure that the API call correctly substitutes {jobId} with the actual jobId value. Verify that the params object is structured as expected by the API client to handle path parameters properly.
Run the following script to verify the API call configuration:
✅ Verification successful
API Path Parameter Usage is Consistent and Correct
The codebase shows consistent usage of path parameters across multiple API calls, including several examples with jobId. The pattern params: { path: { jobId } } is used correctly throughout the SDK and integrations, following the same structure for other path parameters like providerId and agentId. The implementation in github-get-job-inputs/src/index.ts follows this established pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the API client's GET method correctly handles path parameters.
# Test: Review the SDK documentation or source code to confirm the correct usage of path parameters.
# Expected Result: The path parameter `{jobId}` should be replaced with the actual `jobId` value.
# Since we can't execute code, here's how you can verify it manually:
# 1. Open the SDK documentation for the GET method.
# 2. Confirm that passing `params: { path: { jobId } }` correctly replaces `{jobId}` in the URL.
Length of output: 0
Script:
#!/bin/bash
# Let's check the API client implementation and usage patterns in the codebase
# First, let's find the API client implementation
rg -A 5 "\.GET\(.*jobId.*\)" --type ts
# Then, let's look for similar API usage patterns
rg -A 5 "params: \{ path:" --type ts
# Finally, let's check if there are any tests validating this behavior
fd "test|spec" -e ts -x rg -l "GET.*jobId"
Length of output: 2367
apps/webservice/src/app/api/v1/jobs/[jobId]/route.ts (1)
66-69: Verify that extra.params.jobId correctly accesses the job ID
Ensure that extra.params.jobId correctly retrieves the jobId parameter in the authorization middleware. If params is not available in extra, the permission check may not function as intended.
Consider verifying the middleware setup to ensure params are passed to extra. If params are not included, adjust the middleware or the way jobId is accessed.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
github/get-job-inputs/index.js (1)
28044-28046: Fix inconsistent error message terminologyThe error message refers to "Scanner" but the variable is named "scanner", which could be confusing.
- if (scanner == null) throw new Error("Scanner not found"); + if (scanner == null) throw new Error("Target provider not found");
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
github/get-job-inputs/index.js(6 hunks)
🔥 Files not summarized due to errors (1)
- github/get-job-inputs/index.js: Error: Server error: no LLM provider could handle the message
🔇 Additional comments (3)
github/get-job-inputs/index.js (3)
27415-27991: Well-structured API client implementation!
The implementation follows best practices with proper parameter serialization, middleware support, and comprehensive error handling.
28114-28115: Secure configuration implementation!
The SDK client is properly initialized with required inputs for base URL and API key.
28062-28064: Verify the use of PATCH method for retrieval operation
The endpoint uses PATCH for what appears to be a GET operation. PATCH is typically used for partial updates, not for retrieving data.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
integrations/github-get-job-inputs/src/index.ts (2)
46-49: Enhance error message with more context.While the undefined check is good, the error message could be more descriptive to aid in debugging.
Consider this improvement:
- core.error(`Invalid Job data`); + core.error(`Invalid Job data: No data returned for jobId ${jobId}`);
59-59: Consider adding type safety for destructured properties.The destructuring assumes the existence of these properties. Consider adding type guards or default values to handle potential undefined properties safely.
Example improvement:
const { variables = {}, target = null, release = null, environment = null, runbook = null, deployment = null, approval = null, } = data;github/get-job-inputs/index.js (1)
28114-28117: Consider implementing API key security measures.While the implementation works, consider adding security measures for the API key:
- Mask the API key in logs and error messages
- Consider implementing token rotation or expiration mechanisms
const api = dist_createClient({ baseUrl: core.getInput("base_url", { required: true }), - apiKey: core.getInput("api_key", { required: true }), + apiKey: core.getSecret("api_key", { required: true }), // Use core.setSecret to mask the API key });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
github/get-job-inputs/index.js(6 hunks)integrations/github-get-job-inputs/src/index.ts(3 hunks)
🔥 Files not summarized due to errors (1)
- github/get-job-inputs/index.js: Error: Server error: no LLM provider could handle the message
🧰 Additional context used
📓 Learnings (1)
integrations/github-get-job-inputs/src/index.ts (1)
Learnt from: zacharyblasczyk
PR: ctrlplanedev/ctrlplane#196
File: integrations/github-get-job-inputs/src/index.ts:39-39
Timestamp: 2024-11-03T00:15:33.442Z
Learning: In `integrations/github-get-job-inputs/src/sdk.ts`, the API client is properly configured with the `baseUrl` input.
🔇 Additional comments (2)
integrations/github-get-job-inputs/src/index.ts (1)
38-38: LGTM! Type annotation improves code safety.
The explicit type annotation for jobId enhances type safety and code clarity.
github/get-job-inputs/index.js (1)
27997-28108: LGTM! Well-structured SDK implementation.
The SDK implementation demonstrates good practices with proper error handling, caching mechanisms, and clear separation of concerns.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
integrations/github-get-job-inputs/src/index.ts (1)
46-49: Enhance error message with job ID context.While the undefined check is good, the error message could be more helpful by including the job ID.
- core.error(`Invalid Job data`); + core.error(`Invalid Job data for job ID: ${jobId}`);github/get-job-inputs/index.js (2)
28114-28116: Consider making the base URL more configurableThe base URL has a hardcoded fallback to "https://app.ctrlplane.dev". Consider moving this default to a configuration constant or environment variable for better maintainability.
+const DEFAULT_BASE_URL = "https://app.ctrlplane.dev"; + const api = dist_createClient({ - baseUrl: core.getInput("base_url") || "https://app.ctrlplane.dev", + baseUrl: core.getInput("base_url") || DEFAULT_BASE_URL, apiKey: core.getInput("api_key", { required: true }), });
28158-28161: Improve error handling for invalid job dataThe error message for invalid job data is not descriptive enough. Consider adding more context about what makes the data invalid.
- if (data == undefined) { - core.error(`Invalid Job data`); - return; + if (!data) { + core.error(`Invalid Job data: Received ${data}`); + core.setFailed('Failed to fetch valid job data'); + return;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
github/get-job-inputs/index.js(6 hunks)integrations/github-get-job-inputs/src/index.ts(3 hunks)integrations/github-get-job-inputs/src/sdk.ts(1 hunks)
🔥 Files not summarized due to errors (1)
- github/get-job-inputs/index.js: Error: Server error: no LLM provider could handle the message
🚧 Files skipped from review as they are similar to previous changes (1)
- integrations/github-get-job-inputs/src/sdk.ts
🧰 Additional context used
📓 Learnings (1)
integrations/github-get-job-inputs/src/index.ts (1)
Learnt from: zacharyblasczyk
PR: ctrlplanedev/ctrlplane#196
File: integrations/github-get-job-inputs/src/index.ts:39-39
Timestamp: 2024-11-03T00:15:33.442Z
Learning: In `integrations/github-get-job-inputs/src/sdk.ts`, the API client is properly configured with the `baseUrl` input.
🔇 Additional comments (5)
integrations/github-get-job-inputs/src/index.ts (3)
38-38: LGTM! Type safety improvement.
The explicit string type annotation for jobId enhances type safety.
59-59: LGTM! Clean destructuring pattern.
The destructuring matches the expected API response structure and maintains good code readability.
39-39: Verify the default baseUrl value.
The code now provides a default value for baseUrl. While this improves usability, please ensure that "https://app.ctrlplane.dev" is the correct production URL.
✅ Verification successful
The default baseUrl value is correctly set to "https://app.ctrlplane.dev"
The verification shows this URL is consistently used across the codebase:
- As the default API endpoint in multiple integration files
- As the production URL in the documentation (README.md and config-files.mdx)
- As the base URL for the Ctrlplane Cloud service
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other occurrences of the baseUrl to verify consistency
rg -l 'app\.ctrlplane\.dev'
Length of output: 197
Script:
#!/bin/bash
# Let's check the actual content of these files to verify the context of the URL usage
rg -A 2 -B 2 'app\.ctrlplane\.dev'
Length of output: 2110
github/get-job-inputs/index.js (2)
27415-27991: Well-structured OpenAPI client implementation!
The implementation follows best practices with proper parameter validation, comprehensive error handling, and middleware support. The code is modular and maintainable.
28154-28156: Previous error handling comment is still valid
The API call still lacks proper error handling as mentioned in the past review comment.
Summary by CodeRabbit
New Features
Refactor
Chores
sdk.tsfile for API client setup.