-
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
Dev #17
Conversation
configuration.
attempt and core.getInput in action
WalkthroughThe changes introduce a new workflow in the GitHub Actions CI/CD pipeline, which now requires additional data input. The code has been refactored to accommodate this change, with new import statements and function calls. Several new modules have been added, each exporting a Changes
TipsChat with CodeRabbit Bot (
|
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: 9
Configuration used: CodeRabbit UI
Files ignored due to filter (8)
- action.yml
- package.json
- packages/action/package.json
- packages/action/tsconfig.json
- packages/worker/package.json
- packages/worker/tsconfig.json
- pnpm-lock.yaml
- tsconfig.json
Files selected for processing (18)
- .github/workflows/wraith-ci.yml (2 hunks)
- packages/action/src/index.ts (1 hunks)
- packages/ghost/actions.ts (1 hunks)
- packages/ghost/app/build.ts (1 hunks)
- packages/ghost/app/closer.ts (1 hunks)
- packages/ghost/app/deploy.ts (1 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)
- packages/ghost/apps.ts (1 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)
- packages/types/WraithPayload.ts (1 hunks)
- packages/worker/src/index.ts (1 hunks)
Files skipped from review due to trivial changes (6)
- packages/ghost/app/closer.ts
- packages/ghost/app/release.ts
- packages/ghost/apps.ts
- packages/ghost/workers.ts
- packages/types/GhostPayload.ts
- packages/types/WraithPayload.ts
Additional comments: 15
packages/types/WorkerContext.ts (1)
- 1-10: The new
WorkerContext
type is well defined and imports necessary types from the 'octoflare' and 'octoflare/webhook' modules. Ensure that the properties ofWorkerContext
align with the expected usage in the rest of the codebase.packages/ghost/app/merge.ts (2)
1-8: The
worker
andaction
functions are currently empty. Ensure that the implementation for these functions is added in future commits. If these functions are not intended to do anything, consider adding a comment to clarify this.1-1: Ensure that the
Ghost
type is correctly defined in the@/types/Ghost.js
module and that it matches the structure of themerge
object..github/workflows/wraith-ci.yml (2)
5-9: The new input "data" is now required for the workflow. Ensure that all calls to this workflow include the "data" input.
27-30: The "wraith-ci" job now requires the "data" input. Ensure that the "wraith-ci" action can handle this new input correctly.
packages/worker/src/index.ts (2)
1-1: Ensure that the
workers
module is correctly exported from the@/ghost/workers.js
file.44-46: The status code 202 is used to indicate that the request has been accepted for processing, but the processing has not been completed. Ensure that this is the intended use case.
packages/types/Ghost.ts (2)
1-3: New import statements are added for
workers
,GhostPayload
, andWorkerContext
. Ensure that these modules exist and are correctly exported from their respective files.8-10: The type of
data
is inferred using a conditional type based on the return type ofworkers
. This could lead to issues if the return type ofworkers
is not aRecord<string, unknown>
. Ensure that the return type ofworkers
is as expected.packages/action/src/index.ts (3)
1-5: The new import statements look fine. Ensure that the imported modules are installed and available in the project.
7-17: The
action
function is used to wrap the code. Theattempt
function is used to parse the input data as aWraithPayload
object. If parsing fails, an error is thrown. The parsed data is then passed to theactions
function. This is a good use of error handling and asynchronous programming.16-16: Ensure that the
actions
function can handle theWraithPayload
object correctly and that it is implemented to handle any errors that may occur during its execution.packages/ghost/actions.ts (3)
1-2: New import statements for
WraithPayload
andapps
are added. Ensure that these modules exist and are correctly exported.4-17: The
actions
function now accepts aWraithPayload | Error
parameter and usesPromise.allSettled
to iterate over the entries of theapps
object, calling theaction
function of each app. The argument passed to theaction
function is determined based on the type of thepayload
parameter. If it is an instance ofError
, the argument is an object with astatus
of'error'
and thepayload
as theresult
. Otherwise, it uses the corresponding property of thepayload
object. The return value ofactions
is a promise that resolves when all the app actions have settled. This is a significant change to the function's behavior and may affect all parts of the codebase where it is used.8-15: The
action
function of each app is called with a specific argument based on the type of thepayload
parameter. Ifpayload
is an instance ofError
, the argument is an object with astatus
of'error'
and thepayload
as theresult
. Otherwise, it uses the corresponding property of thepayload
object. Ensure that theaction
function of each app can handle these two types of arguments correctly.
import { Ghost } from '@/types/Ghost.js' | ||
|
||
export const format: Ghost = { | ||
worker: async (context) => { | ||
}, | ||
action: async (data) => { | ||
} | ||
} |
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 worker
and action
functions are currently empty. Ensure that the implementation for these functions is added before merging the PR. If these functions are intentionally left empty, consider adding a comment to explain why.
worker: async (context) => {
+ // Implementation goes here
},
action: async (data) => {
+ // Implementation goes here
}
Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Make sure it correctly replaces the highlighted code, has no missing lines and indentaion issues.
import { Ghost } from '@/types/Ghost.js' | |
export const format: Ghost = { | |
worker: async (context) => { | |
}, | |
action: async (data) => { | |
} | |
} | |
import { Ghost } from '@/types/Ghost.js' | |
export const format: Ghost = { | |
worker: async (context) => { | |
// Implementation goes here | |
}, | |
action: async (data) => { | |
// Implementation goes here | |
} | |
} |
import { Ghost } from '@/types/Ghost.js' | ||
|
||
export const deploy: Ghost = { | ||
worker: async (context) => { | ||
}, | ||
action: async (data) => { | ||
} | ||
} |
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 worker
and action
functions are currently empty. Ensure that the implementation for these functions is added in future commits. If these functions are not intended to have any logic, consider removing them to avoid confusion.
- worker: async (context) => {
- },
- action: async (data) => {
- }
+ // worker: async (context) => {
+ // },
+ // action: async (data) => {
+ // }
Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Make sure it correctly replaces the highlighted code, has no missing lines and indentaion issues.
import { Ghost } from '@/types/Ghost.js' | |
export const deploy: Ghost = { | |
worker: async (context) => { | |
}, | |
action: async (data) => { | |
} | |
} | |
import { Ghost } from '@/types/Ghost.js' | |
export const deploy: Ghost = { | |
// worker: async (context) => { | |
// }, | |
// action: async (data) => { | |
// } | |
} |
import { Ghost } from '@/types/Ghost.js' | ||
|
||
export const build: Ghost = { | ||
worker: async (context) => { | ||
}, | ||
action: async (data) => { | ||
} | ||
} |
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 worker
and action
functions are currently empty. Ensure that the implementation is added in future commits. If these functions are not intended to do anything, consider adding a comment to clarify this.
export const build: Ghost = {
+ // This function is intentionally left blank
worker: async (context) => {
},
+ // This function is intentionally left blank
action: async (data) => {
}
}
Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Make sure it correctly replaces the highlighted code, has no missing lines and indentaion issues.
import { Ghost } from '@/types/Ghost.js' | |
export const build: Ghost = { | |
worker: async (context) => { | |
}, | |
action: async (data) => { | |
} | |
} | |
export const build: Ghost = { | |
// This function is intentionally left blank | |
worker: async (context) => { | |
}, | |
// This function is intentionally left blank | |
action: async (data) => { | |
} | |
} |
import { Ghost } from '@/types/Ghost.js' | ||
|
||
export const docs: Ghost = { | ||
worker: async (context) => { | ||
}, | ||
action: async (data) => { | ||
} | ||
} |
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 worker
and action
functions are currently empty. Ensure that the implementation for these functions is added before merging the PR. If these functions are placeholders for future implementation, consider adding a comment to indicate this.
export const docs: Ghost = {
- worker: async (context) => {
+ worker: async (context) => {
+ // TODO: Implement worker function
},
- action: async (data) => {
+ action: async (data) => {
+ // TODO: Implement action function
}
}
Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Make sure it correctly replaces the highlighted code, has no missing lines and indentaion issues.
import { Ghost } from '@/types/Ghost.js' | |
export const docs: Ghost = { | |
worker: async (context) => { | |
}, | |
action: async (data) => { | |
} | |
} | |
export const docs: Ghost = { | |
worker: async (context) => { | |
// TODO: Implement worker function | |
}, | |
action: async (data) => { | |
// TODO: Implement action function | |
} | |
} |
export default octoflare(async ({ payload, installation }) => { | ||
if (!installation) { | ||
return new Response('Skip Event: No Installation', { | ||
return new Response('No Installation', { | ||
status: 200 | ||
}) | ||
} | ||
|
||
if (!('commits' in payload)) { | ||
return new Response('Skip Event: No Push Event', { | ||
if (!('repository' in payload && payload.repository)) { | ||
return new Response('No Repository', { | ||
status: 200 | ||
}) | ||
} | ||
|
||
const { repository, after } = payload | ||
const { repository } = payload | ||
|
||
if (Number(after) === 0) { | ||
return new Response('Skip Event: Head SHA = 0', { | ||
const repo = repository.name | ||
const owner = repository.owner.login | ||
|
||
const data = await workers({ | ||
repo, | ||
owner, | ||
payload, | ||
repository, | ||
installation | ||
}) | ||
|
||
if (!data) { | ||
return new Response('No Wraith CI Payload', { | ||
status: 200 | ||
}) | ||
} | ||
|
||
const { dispatchWorkflow } = await installation.createCheckRun({ | ||
repo: repository.name, | ||
owner: repository.owner.login, | ||
name: 'Wraith CI', | ||
head_sha: after | ||
await installation.startWorkflow({ | ||
payload: { | ||
repo, | ||
owner | ||
}, | ||
data: JSON.stringify(data) | ||
}) | ||
|
||
await dispatchWorkflow() | ||
|
||
return new Response('Wraith CI Workflow Dispatched', { | ||
return new Response('Wraith CI Workflow Submitted', { | ||
status: 202 | ||
}) | ||
}) |
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 function is missing error handling. Consider wrapping the function body in a try-catch block to handle potential errors.
+ try {
// function body
+ } catch (error) {
+ console.error(error);
+ return new Response('An error occurred', { status: 500 });
+ }
Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Make sure it correctly replaces the highlighted code, has no missing lines and indentaion issues.
export default octoflare(async ({ payload, installation }) => { | |
if (!installation) { | |
return new Response('Skip Event: No Installation', { | |
return new Response('No Installation', { | |
status: 200 | |
}) | |
} | |
if (!('commits' in payload)) { | |
return new Response('Skip Event: No Push Event', { | |
if (!('repository' in payload && payload.repository)) { | |
return new Response('No Repository', { | |
status: 200 | |
}) | |
} | |
const { repository, after } = payload | |
const { repository } = payload | |
if (Number(after) === 0) { | |
return new Response('Skip Event: Head SHA = 0', { | |
const repo = repository.name | |
const owner = repository.owner.login | |
const data = await workers({ | |
repo, | |
owner, | |
payload, | |
repository, | |
installation | |
}) | |
if (!data) { | |
return new Response('No Wraith CI Payload', { | |
status: 200 | |
}) | |
} | |
const { dispatchWorkflow } = await installation.createCheckRun({ | |
repo: repository.name, | |
owner: repository.owner.login, | |
name: 'Wraith CI', | |
head_sha: after | |
await installation.startWorkflow({ | |
payload: { | |
repo, | |
owner | |
}, | |
data: JSON.stringify(data) | |
}) | |
await dispatchWorkflow() | |
return new Response('Wraith CI Workflow Dispatched', { | |
return new Response('Wraith CI Workflow Submitted', { | |
status: 202 | |
}) | |
}) | |
export default octoflare(async ({ payload, installation }) => { | |
try { | |
if (!installation) { | |
return new Response('No Installation', { | |
status: 200 | |
}) | |
} | |
if (!('repository' in payload && payload.repository)) { | |
return new Response('No Repository', { | |
status: 200 | |
}) | |
} | |
const { repository } = payload | |
const repo = repository.name | |
const owner = repository.owner.login | |
const data = await workers({ | |
repo, | |
owner, | |
payload, | |
repository, | |
installation | |
}) | |
if (!data) { | |
return new Response('No Wraith CI Payload', { | |
status: 200 | |
}) | |
} | |
await installation.startWorkflow({ | |
payload: { | |
repo, | |
owner | |
}, | |
data: JSON.stringify(data) | |
}) | |
return new Response('Wraith CI Workflow Submitted', { | |
status: 202 | |
}) | |
} catch (error) { | |
console.error(error); | |
return new Response('An error occurred', { status: 500 }); | |
} | |
}) |
import { Ghost } from '@/types/Ghost.js' | ||
|
||
export const lint: Ghost = { | ||
worker: async (context) => { | ||
}, | ||
action: async (data) => { | ||
} | ||
} |
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 worker
and action
functions are currently empty. If this is intentional and they are to be implemented later, consider adding a comment or a throw new Error("Not implemented")
statement to make it clear that these are placeholders. If they are not supposed to be empty, ensure to implement the necessary logic.
export const lint: Ghost = {
- worker: async (context) => {
+ worker: async (context) => {
+ throw new Error("Not implemented");
},
- action: async (data) => {
+ action: async (data) => {
+ throw new Error("Not implemented");
}
}
Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Make sure it correctly replaces the highlighted code, has no missing lines and indentaion issues.
import { Ghost } from '@/types/Ghost.js' | |
export const lint: Ghost = { | |
worker: async (context) => { | |
}, | |
action: async (data) => { | |
} | |
} | |
import { Ghost } from '@/types/Ghost.js' | |
export const lint: Ghost = { | |
worker: async (context) => { | |
throw new Error("Not implemented"); | |
}, | |
action: async (data) => { | |
throw new Error("Not implemented"); | |
} | |
} |
await installation.startWorkflow({ | ||
payload: { | ||
repo, | ||
owner | ||
}, | ||
data: JSON.stringify(data) | ||
}) |
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 startWorkflow
method is being called without checking if it exists on the installation
object. Consider adding a check before calling the method.
+ if (typeof installation.startWorkflow === 'function') {
await installation.startWorkflow({
payload: {
repo,
owner
},
data: JSON.stringify(data)
});
+ } else {
+ console.error('startWorkflow method does not exist on installation object');
+ return new Response('An error occurred', { status: 500 });
+ }
Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Make sure it correctly replaces the highlighted code, has no missing lines and indentaion issues.
await installation.startWorkflow({ | |
payload: { | |
repo, | |
owner | |
}, | |
data: JSON.stringify(data) | |
}) | |
if (typeof installation.startWorkflow === 'function') { | |
await installation.startWorkflow({ | |
payload: { | |
repo, | |
owner | |
}, | |
data: JSON.stringify(data) | |
}); | |
} else { | |
console.error('startWorkflow method does not exist on installation object'); | |
return new Response('An error occurred', { status: 500 }); | |
} |
export type Ghost = { | ||
worker: (context: WorkerContext) => Promise<GhostPayload | void> | ||
action?: ( | ||
data: Awaited<ReturnType<typeof workers>> extends Record<string, infer U> | ||
? U | ||
: never | ||
) => Promise<unknown> | ||
} |
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 Ghost
type is defined with two properties: worker
and action
. The worker
function takes a WorkerContext
parameter and returns a Promise
that resolves to either a GhostPayload
or void
. The action
function is optional and takes a data
parameter, the type of which is inferred based on the return type of the workers
function. This is a complex type inference and could lead to issues if the return type of workers
changes in the future. Consider simplifying this or adding comments to explain the type inference.
() => { | ||
const str = core.getInput('data') | ||
return JSON.parse(str) as WraithPayload | ||
}, | ||
(e, o) => e ?? new Error(String(o)) | ||
) |
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 attempt
function is used to parse the input data as a WraithPayload
object. If parsing fails, an error is thrown. This is a good use of error handling. However, ensure that the core.getInput('data')
function is correctly retrieving the input data.
- const str = core.getInput('data')
+ const str = core.getInput('data') || '{}'
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 str = core.getInput('data') | |
return JSON.parse(str) as WraithPayload | |
}, | |
(e, o) => e ?? new Error(String(o)) | |
) | |
() => { | |
const str = core.getInput('data') || '{}' | |
return JSON.parse(str) as WraithPayload | |
}, | |
(e, o) => e ?? new Error(String(o)) | |
) |
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: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- packages/ghost/utils/index.test.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- packages/ghost/utils/index.test.ts
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation