-
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: increase timeout to the same as build-push-action #11
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 f854f4a in 1 minute and 7 seconds
More details
- Looked at
55
lines of code in2
files - Skipped
2
files when reviewing. - Skipped posting
9
drafted comments based on config settings.
1. src/main.ts:70
- Draft comment:
Changed logging from error to warning in maybeFormatBlockDevice. Verify that non-fatal logging is acceptable here. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
2. src/main.ts:79
- Draft comment:
Timeout increased to stickyDiskTimeoutMs (45s) for mounting. Confirm that 45s is suitable for VM agent operations. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
3. src/main.ts:126
- Draft comment:
Replaced core.setFailed with core.warning on error in run(). Confirm this change aligns with desired error handling. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
4. src/post.ts:109
- Draft comment:
Changed logging level from error to warning on cleanup failure. Verify that silent failure is acceptable here. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
5. src/main.ts:12
- Draft comment:
Consider making stickyDiskTimeoutMs configurable via an input or environment variable instead of a hardcoded 45000. This would improve flexibility for different environments. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
6. src/main.ts:72
- Draft comment:
Downgrading from core.error to core.warning when formatting fails might obscure critical failures. Verify if a warning is sufficient since the error is rethrown. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
7. src/main.ts:79
- Draft comment:
The timeout for mounting the sticky disk has been increased to stickyDiskTimeoutMs. Ensure this longer delay is acceptable and aligns with client expectations. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
8. src/main.ts:134
- Draft comment:
Switching from core.setFailed to core.warning in run() may lead to silent failures if getting the sticky disk is critical. Confirm that this downgrade is intentional. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
9. src/post.ts:109
- Draft comment:
Changing the log from core.error to core.warning during cleanup might mask issues during disk unmount/commit. Ensure that warnings are sufficient to alert users of cleanup failures. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
Workflow ID: wflow_kV7fbypeyJYDyEPX
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
f854f4a
to
5175c4c
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 5175c4c in 1 minute and 3 seconds
More details
- Looked at
68
lines of code in3
files - Skipped
3
files when reviewing. - Skipped posting
11
drafted comments based on config settings.
1. package.json:13
- Draft comment:
Verify new dependency version compatibility with existing code and build-push-action. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is asking the PR author to verify compatibility of a new dependency version, which falls under the rule of not commenting on dependency changes or asking the author to ensure compatibility. It doesn't provide a specific suggestion or point out a specific issue.
2. src/main.ts:12
- Draft comment:
The stickyDiskTimeoutMs constant (45000 ms) now aligns with build-push-action; ensure this timeout suffices for all scenarios. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =20%
<= threshold50%
This comment is asking the author to ensure that the timeout is sufficient for all scenarios, which is against the rules. It doesn't provide a specific suggestion or ask for a specific test to be written. It is more of a general cautionary note.
3. src/main.ts:79
- Draft comment:
Timeout now uses stickyDiskTimeoutMs instead of a hardcoded 15000. Confirm this change won't introduce unexpected delays. - Reason this comment was not posted:
Marked as duplicate.
4. src/main.ts:72
- Draft comment:
Changed logging from error to warning when formatting fails; verify that a warning is sufficient and errors aren't masked. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =40%
<= threshold50%
The comment is asking the PR author to verify if changing the logging level from error to warning is sufficient and if errors aren't being masked. This is asking for confirmation of intention, which violates the rules. However, it does point out a potential issue with the change that could be useful for the author to consider.
5. src/main.ts:134
- Draft comment:
core.setFailed was replaced with core.warning on disk errors; check if the workflow should fail on sticky disk errors. - Reason this comment was not posted:
Marked as duplicate.
6. src/post.ts:109
- Draft comment:
Switching from error to warning in cleanup logs may hide issues; confirm that non-critical failures are acceptable. - Reason this comment was not posted:
Marked as duplicate.
7. package.json:13
- Draft comment:
Dependency version bumped. Confirm that the new version (^1.6.1-20250209182455-7d83cfb8ddb1.2) is fully compatible. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
8. src/main.ts:12
- Draft comment:
Sticky disk timeout increased to 45s. Consider making stickyDiskTimeoutMs configurable if needed. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
9. src/main.ts:72
- Draft comment:
Changed logging from error to warning on formatting failure. Verify if a non-fatal warning is the intended behavior. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
10. src/main.ts:133
- Draft comment:
Switched from core.setFailed to core.warning for sticky disk errors. Confirm that failure to get the sticky disk should not cause the action to fail. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
11. src/post.ts:109
- Draft comment:
Changed cleanup error logging from error to warning. Confirm that cleanup failures should not propagate as fatal errors. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
Workflow ID: wflow_H5N1Ssyx3YlDLbwM
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Important
Increased sticky disk mount timeout and changed error handling to warnings in
src/main.ts
andsrc/post.ts
, and updated a dependency version inpackage.json
.src/main.ts
, increased timeout formountStickyDisk()
from 15000ms tostickyDiskTimeoutMs
(45000ms).core.error
tocore.warning
inmaybeFormatBlockDevice()
andrun()
insrc/main.ts
.core.error
tocore.warning
inrun()
insrc/post.ts
.@buf/blacksmith_vm-agent.connectrpc_es
version inpackage.json
.This description was created by
for 5175c4c. It will automatically update as commits are pushed.