-
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
src: fix the movement of cleanup to the main step #48
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.
👍 Looks good to me! Reviewed everything up to 4332263 in 23 seconds
More details
- Looked at
271
lines of code in1
files - Skipped
2
files when reviewing. - Skipped posting
5
drafted comments based on config settings.
1. src/main.ts:40
- Draft comment:
The functionreportBuildCompleted
now takes additional parameters (blacksmithDockerBuildId
,buildRef
,dockerBuildDurationSeconds
), which improves its flexibility and clarity. - Reason this comment was not posted:
Confidence changes required:0%
The functionreportBuildCompleted
now takes additional parameters which are passed correctly. This change improves the function's flexibility and clarity.
2. src/main.ts:94
- Draft comment:
The functionreportBuildFailed
now takes additional parameters (dockerBuildId
,dockerBuildDurationSeconds
), which improves its flexibility and clarity. - Reason this comment was not posted:
Confidence changes required:0%
The functionreportBuildFailed
now takes additional parameters which are passed correctly. This change improves the function's flexibility and clarity.
3. src/main.ts:401
- Draft comment:
The functiongetBuilderAddr
now returns an object withaddr
andbuildId
, which improves clarity and future extensibility. - Reason this comment was not posted:
Confidence changes required:0%
The functiongetBuilderAddr
now returns an object withaddr
andbuildId
, which is a good change for clarity and future extensibility.
4. src/main.ts:518
- Draft comment:
The change fromlocalBuilderAddr
tobuilderInfo
is a good refactor for clarity and to accommodate additional information likebuildId
. - Reason this comment was not posted:
Confidence changes required:0%
The change fromlocalBuilderAddr
tobuilderInfo
is a good refactor for clarity and to accommodate additional information likebuildId
.
5. src/main.ts:765
- Draft comment:
The removal of the build summary generation in the post step is a significant change. Ensure that this functionality is not needed elsewhere or is handled differently. - Reason this comment was not posted:
Confidence changes required:50%
The removal of the build summary generation in the post step is a significant change. Ensure that this functionality is not needed elsewhere or is handled differently.
Workflow ID: wflow_10U8mdx0DTu2tC0a
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
⌛ 1 day left in your free trial, upgrade for $20/seat/month or contact us.
4332263
to
b80dd68
Compare
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.
👍 Looks good to me! Incremental review on b80dd68 in 22 seconds
More details
- Looked at
279
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. src/main.ts:645
- Draft comment:
Ensure that all instances ofstateHelper
related todockerBuildDurationSeconds
andblacksmithDockerBuildId
are removed or updated to use the new parameterized approach to avoid inconsistencies. - Reason this comment was not posted:
Comment did not seem useful.
2. src/main.ts:445
- Draft comment:
Ensure consistent handling of thebuildId
returned bygetBuilderAddr
throughout the code to prevent potential issues. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_KixIshBsbncQ0VEl
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
⌛ 1 day left in your free trial, upgrade for $20/seat/month or contact us.
b80dd68
to
20b0276
Compare
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.
👍 Looks good to me! Incremental review on 20b0276 in 18 seconds
More details
- Looked at
285
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. src/main.ts:16
- Draft comment:
TheUploadArtifactResponse
import is still present but should be removed as per the PR description. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_4LML8wsvmvEiu5NH
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
⌛ 1 day left in your free trial, upgrade for $20/seat/month or contact us.
20b0276
to
1dee25c
Compare
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.
👍 Looks good to me! Incremental review on 1dee25c in 22 seconds
More details
- Looked at
288
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. src/main.ts:770
- Draft comment:
The cleanup process is still present in the post step, which seems contrary to the PR description stating that cleanup is moved to the main step. Please ensure this is intentional or update the description accordingly. - Reason this comment was not posted:
Comment did not seem useful.
2. src/main.ts:714
- Draft comment:
The code still checks for build summary support, which seems contrary to the PR description stating that build summary generation is removed. Please ensure this is intentional or update the description accordingly. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_igGBeKNYscGwmxMS
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
⌛ 1 day left in your free trial, upgrade for $20/seat/month or contact us.
|
Important
Refactor build reporting functions to use parameters and update cleanup process in
main.ts
.reportBuildCompleted
andreportBuildFailed
now takeblacksmithDockerBuildId
anddockerBuildId
as parameters instead of usingstateHelper
.getBuilderAddr
return type changed to includebuildId
.reportBuildCompleted
andreportBuildFailed
updated to use passed parameters.UploadArtifactResponse
import frommain.ts
.This description was created by for 1dee25c. It will automatically update as commits are pushed.