-
Notifications
You must be signed in to change notification settings - Fork 0
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
Refactor worker and action functions to include CloseCheckParam #33
Conversation
GhostPayload types.
access tokens.
WalkthroughThe changes primarily revolve around the modification of function signatures and the introduction of asynchronous operations in the Changes
Poem
TipsChat with CodeRabbit Bot (
|
Deploying with Cloudflare Pages
|
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.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (6)
- packages/action/src/index.ts (2 hunks)
- packages/ghost/actions.ts (2 hunks)
- packages/ghost/workers.ts (1 hunks)
- packages/types/Ghost.ts (1 hunks)
- packages/types/GhostPayload.ts (1 hunks)
- packages/types/WorkerContext.ts (1 hunks)
Additional comments: 14
packages/types/GhostPayload.ts (1)
- 1-3: The
check_run_id
property is now required in theGhostPayload
type. Ensure that all instances whereGhostPayload
is used are updated to include this property.packages/types/Ghost.ts (2)
1-1: The new import statement for
CloseCheckParam
is correctly added.8-8: The return type of the
action
function has been changed toPromise<CloseCheckParam>
. Ensure that all implementations of this function are updated to return aPromise<CloseCheckParam>
.packages/types/WorkerContext.ts (2)
8-8: The
head_sha
property has been added. Ensure that all instances whereWorkerContext
is used have been updated to handle this new property.12-12: The
createCheckRun
method has been added. Ensure that all instances whereWorkerContext
is used have been updated to handle this new method.packages/action/src/index.ts (3)
8-11: The function
action
now accepts a new parameterappkit
. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.29-30: The installation access token is being revoked for both
octokit
andappkit
. This is a good security practice as it minimizes the risk of token misuse.28-31: The
Promise.all
function is used to execute multiple promises concurrently. This is a good practice for improving performance. However, if one promise fails,Promise.all
fails with the reason of the first promise that rejected. Make sure to handle any potential errors that might occur during the execution of these promises.packages/ghost/actions.ts (3)
1-5: Ensure that the newly added imports are being used correctly and that they are necessary for the code. Also, verify that the imported modules are installed in your project dependencies.
8-11: The
actions
function has been modified to handle asynchronous operations. Ensure that all calls to this function throughout the codebase have been updated to handle the Promise returned by this function.55-82: Error handling has been added to the
actions
function. Ensure that the error messages are meaningful and helpful for debugging. Also, verify that thecore.setFailed
function is being used correctly.packages/ghost/workers.ts (3)
5-7: The function signature has been updated to exclude 'createCheckRun' and 'head_sha' from the WorkerContext. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.
36-55: The 'attempt' function is used to handle potential errors. This is a good practice as it improves the robustness of the code.
60-60: The 'Promise.all' function is used to handle multiple promises. This is a good practice as it improves the performance of the code by allowing multiple operations to be performed concurrently.
packages/ghost/actions.ts
Outdated
const closeCheckRun = async (param: CloseCheckParam) => { | ||
const { status, result } = data | ||
|
||
if (status === 'error') { | ||
return | ||
} | ||
|
||
const check_run_id = result.check_run_id | ||
|
||
if (!check_run_id) { | ||
return | ||
} | ||
|
||
const outputs = | ||
typeof param === 'string' ? { conclusion: param } : param | ||
|
||
await octokit.rest.checks.update({ | ||
check_run_id, | ||
owner, | ||
repo, | ||
details_url, | ||
...outputs | ||
}) | ||
} |
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.
The closeCheckRun
function is defined within the actions
function. This could lead to the function being redefined every time actions
is called, which is not efficient. Consider moving closeCheckRun
outside of actions
to improve performance.
+ const closeCheckRun = async (param: CloseCheckParam, data: any, octokit: any, owner: any, repo: any, details_url: any) => {
- const closeCheckRun = async (param: CloseCheckParam) => {
const { status, result } = data
// ...
await octokit.rest.checks.update({
check_run_id,
owner,
repo,
details_url,
...outputs
})
}
- export const actions = (
+ export const actions = async (
context: Omit<ActionContext, 'data'> & { data: WraithPayload | Error }
) => {
// ...
try {
const result = await app.action?.({
...context,
data
})
if (!result) {
return
}
- await closeCheckRun(result)
+ await closeCheckRun(result, data, octokit, owner, repo, details_url)
} catch (e) {
// ...
}
}
Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Make sure it correctly replaces the highlighted code, has no missing lines and indentaion issues.
const closeCheckRun = async (param: CloseCheckParam) => { | |
const { status, result } = data | |
if (status === 'error') { | |
return | |
} | |
const check_run_id = result.check_run_id | |
if (!check_run_id) { | |
return | |
} | |
const outputs = | |
typeof param === 'string' ? { conclusion: param } : param | |
await octokit.rest.checks.update({ | |
check_run_id, | |
owner, | |
repo, | |
details_url, | |
...outputs | |
}) | |
} | |
const closeCheckRun = async (param: CloseCheckParam, data: any, octokit: any, owner: any, repo: any, details_url: any) => { | |
const { status, result } = data | |
if (status === 'error') { | |
return | |
} | |
const check_run_id = result.check_run_id | |
if (!check_run_id) { | |
return | |
} | |
const outputs = | |
typeof param === 'string' ? { conclusion: param } : param | |
await octokit.rest.checks.update({ | |
check_run_id, | |
owner, | |
repo, | |
details_url, | |
...outputs | |
}) | |
} | |
export const actions = async ( | |
context: Omit<ActionContext, 'data'> & { data: WraithPayload | Error } | |
) => { | |
// ... | |
try { | |
const result = await app.action?.({ | |
...context, | |
data | |
}) | |
if (!result) { | |
return | |
} | |
await closeCheckRun(result, data, octokit, owner, repo, details_url) | |
} catch (e) { | |
// ... | |
} | |
} |
const head_sha = | ||
'pull_request' in payload | ||
? payload.pull_request.head.sha | ||
: 'after' in payload | ||
? payload.after | ||
: null |
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.
The logic for determining the 'head_sha' value is not clear. Consider adding comments to explain the logic behind these checks.
const createCheckRun = async (name: string) => { | ||
if (!(head_sha && Number(head_sha) !== 0)) { | ||
return 0 | ||
} | ||
|
||
const { | ||
data: { id } | ||
} = await installation.kit.rest.checks.create({ | ||
repo, | ||
owner, | ||
name, | ||
await attempt( | ||
async () => { | ||
const result = await app.worker(context) | ||
return result | ||
? { | ||
status: 'success' as const, | ||
result | ||
} | ||
: null | ||
}, | ||
(e, o) => ({ | ||
status: 'error' as const, | ||
result: e ?? new Error(String(o)) | ||
}) | ||
) | ||
] as const | ||
) | ||
head_sha, | ||
status: 'in_progress' | ||
}) | ||
|
||
return id | ||
} |
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.
The 'createCheckRun' function is defined within the 'workers' function. This could lead to the function being redefined every time 'workers' is called, which could impact performance. Consider moving 'createCheckRun' outside of 'workers' to avoid this.
+ const createCheckRun = async (name: string, head_sha: string, installation: any, repo: any, owner: any) => {
+ if (!(head_sha && Number(head_sha) !== 0)) {
+ return 0
+ }
+ const {
+ data: { id }
+ } = await installation.kit.rest.checks.create({
+ repo,
+ owner,
+ name,
+ head_sha,
+ status: 'in_progress'
+ })
+ return id
+ }
- const createCheckRun = async (name: string) => {
- if (!(head_sha && Number(head_sha) !== 0)) {
- return 0
- }
- const {
- data: { id }
- } = await installation.kit.rest.checks.create({
- repo,
- owner,
- name,
- head_sha,
- status: 'in_progress'
- })
- return id
- }
Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Make sure it correctly replaces the highlighted code, has no missing lines and indentaion issues.
const createCheckRun = async (name: string) => { | |
if (!(head_sha && Number(head_sha) !== 0)) { | |
return 0 | |
} | |
const { | |
data: { id } | |
} = await installation.kit.rest.checks.create({ | |
repo, | |
owner, | |
name, | |
await attempt( | |
async () => { | |
const result = await app.worker(context) | |
return result | |
? { | |
status: 'success' as const, | |
result | |
} | |
: null | |
}, | |
(e, o) => ({ | |
status: 'error' as const, | |
result: e ?? new Error(String(o)) | |
}) | |
) | |
] as const | |
) | |
head_sha, | |
status: 'in_progress' | |
}) | |
return id | |
} | |
const createCheckRun = async (name: string, head_sha: string, installation: any, repo: any, owner: any) => { | |
if (!(head_sha && Number(head_sha) !== 0)) { | |
return 0 | |
} | |
const { | |
data: { id } | |
} = await installation.kit.rest.checks.create({ | |
repo, | |
owner, | |
name, | |
head_sha, | |
status: 'in_progress' | |
}) | |
return id | |
} |
lint, merge, and release functions.
run for Ghost Deploy.
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (2)
- package.json
- pnpm-lock.yaml
Files selected for processing (9)
- packages/ghost/actions.ts (2 hunks)
- packages/ghost/app/build.ts (1 hunks)
- packages/ghost/app/closer.ts (1 hunks)
- packages/ghost/app/deploy.ts (4 hunks)
- packages/ghost/app/docs.ts (1 hunks)
- packages/ghost/app/format.ts (1 hunks)
- packages/ghost/app/lint.ts (1 hunks)
- packages/ghost/app/merge.ts (1 hunks)
- packages/ghost/app/release.ts (1 hunks)
Additional comments: 16
packages/ghost/app/format.ts (1)
- 1-5: The
action
function has been removed from theformat
object. Ensure that there are no remaining calls toformat.action
in the codebase.packages/ghost/app/closer.ts (1)
- 1-5: The
action
function has been removed from thecloser
object. Ensure that this function is not being called anywhere else in the codebase.packages/ghost/app/release.ts (1)
- 1-5: Ensure that all references to the
action
function from therelease
object have been updated or removed throughout the codebase. Also, verify that theworker
function is implemented correctly in the future, as it is currently an empty function.packages/ghost/app/build.ts (1)
- 1-5: The
action
function has been removed from thebuild
object. Ensure that this function is not being called anywhere else in the codebase. If it is, those calls will need to be updated or removed.packages/ghost/app/lint.ts (1)
- 1-5: The
action
function has been removed from thelint
object. Ensure that this change does not break any dependencies elsewhere in the codebase. Also, theworker
function is currently an empty async function. If this is intentional, ensure that it is handled properly where it's called to avoid unexpected behavior.packages/ghost/app/merge.ts (1)
- 1-5: The
action
function has been removed from themerge
object. Ensure that this change does not break any dependencies that might be using theaction
function from themerge
object. Also, theworker
function is currently an empty async function. If this is intentional, ignore this comment. Otherwise, you might want to add some functionality to it.packages/ghost/app/docs.ts (1)
- 1-5: The
action
function has been removed from thedocs
object. Ensure that this change does not break any dependencies elsewhere in the codebase. Also, theworker
function is currently an empty async function. If this is intentional, ensure that it is handled properly where it's called to avoid unexpected behavior.packages/ghost/actions.ts (5)
1-5: Ensure that the new imports are used correctly and that they don't conflict with existing imports.
8-10: The function signature has been updated. Make sure all calls to this function have been updated accordingly.
18-85: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [11-85]
The function now handles asynchronous operations and includes error handling logic. This is a significant change in behavior and should be thoroughly tested.
30-54: The new
closeCheckRun
function handles the closing of check runs based on the result. Ensure that this function is called correctly and that it handles all possible results.56-83: The
app.action
call now includes error handling and sets the check run status accordingly. This is a significant change in behavior and should be thoroughly tested.packages/ghost/app/deploy.ts (4)
3-17: The
worker
function signature has been updated to includecreateCheckRun
and removerepo
andowner
. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.25-29: The creation of check run using
installation.kit.rest.checks.create
has been replaced with a call tocreateCheckRun
method. This change improves modularity and makes the code easier to test.31-34: The
action
function now returns 'failure' ifstatus
is 'error'. This change improves error handling.41-55: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [44-65]
The
action
function now returns 'success' ifexitCode
is 0, otherwise it returns an object withconclusion
as 'failure' andoutput
containing error details. This change improves error handling and provides more detailed feedback in case of failure.
Refactor worker and action functions to include CloseCheckParam and update Ghost and GhostPayload types. Import CloseCheckParam and update actions function. Add appkit to payload and revoke installation access tokens. This PR improves the functionality of the worker and action functions by including CloseCheckParam and updating the Ghost and GhostPayload types. It also adds appkit to the payload and revokes installation access tokens for improved security.
Summary by CodeRabbit