-
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: change sticky disk key to repo name #50
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 222c008 in 27 seconds
More details
- Looked at
34
lines of code in1
files - Skipped
2
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. src/main.ts:424
- Draft comment:
ThedockerfilePath
parameter is no longer used in thegetBuilderAddr
function after the changes. Consider removing it to avoid confusion. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_Y7YWfORyMPAkaHRr
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.
222c008
to
bda6587
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 bda6587 in 22 seconds
More details
- Looked at
37
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. src/main.ts:427
- Draft comment:
Ensurestickydisk_key
is consistent with the new sticky disk key logic. Consider usingrepoName
instead ofstickydiskKey
. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_owzJzfkprAKKwXZ1
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.
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 9b63433 in 11 seconds
More details
- Looked at
13
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. src/main.ts:574
- Draft comment:
Consider checking if there are any length restrictions on builder names. UsingDate.now().toString(36)
may result in long names, which could cause issues if there are such restrictions. - Reason this comment was not posted:
Confidence changes required:50%
The change in line 574 introduces a potential issue with the builder name. UsingDate.now().toString(36)
ensures uniqueness but may lead to excessively long names, which could cause issues if there are length restrictions on builder names.
Workflow ID: wflow_ZXbGgut4Ot4sbrgY
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
Change
getStickyDisk
insrc/main.ts
to useGITHUB_REPO_NAME
as sticky disk key instead ofdockerfilePath
.getStickyDisk
to useGITHUB_REPO_NAME
asstickyDiskKey
instead ofdockerfilePath
insrc/main.ts
.getStickyDisk
to reflect the use ofrepoName
.getStickyDisk
ingetBuilderAddr
to removedockerfilePath
argument.This description was created by for 9b63433. It will automatically update as commits are pushed.