internal: fix and improve get-logs GitHub workflow #2201
Conversation
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to 3fe4526 in 7 seconds
More details
- Looked at
10lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. misc/deploy/get_logs_from_server.sh:42
- Draft comment:
The change correctly adds quotes around variables to prevent word splitting and globbing issues. This is a good practice in shell scripting. - Reason this comment was not posted:
Confidence changes required:0%
The change in the PR is correct and improves the script by properly quoting variables to prevent word splitting and globbing issues. This is a good practice in shell scripting.
Workflow ID: wflow_Cpz2COmyfYEPwPd2
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on b1d18a4 in 9 seconds
More details
- Looked at
22lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. .github/workflows/get-logs.yml:39
- Draft comment:
Ensureget_logs_from_server.shcorrectly handles spaces and special characters inDEPLOY_BRANCHandGREP_ARGvariables, as mentioned in the PR description. - Reason this comment was not posted:
Confidence changes required:50%
The PR description mentions fixing quoting issues inget_logs_from_server.sh, but the script itself is not visible in the diff. However, the workflow file is updated to use the script. It's important to ensure that the script handles spaces and special characters in variables correctly.
Workflow ID: wflow_muXRKEf6rk7G4sAM
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 4a0d3ec in 15 seconds
More details
- Looked at
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. .github/workflows/get-logs.yml:46
- Draft comment:
The change frompath: misc/deploytopath: ./misc/deployis unnecessary. The./prefix is redundant and can be omitted for clarity. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment addresses a change made in the diff, which is the addition of the./prefix. The comment claims that this change is unnecessary. The./prefix is often used to denote the current directory, but in many contexts, it is indeed redundant. The comment is actionable as it suggests removing the./prefix for clarity.
The comment might be considered unimportant if the./prefix does not affect functionality. However, it does address a change made in the diff and provides a clear suggestion.
Even if the./prefix is functionally redundant, the comment provides a clear and actionable suggestion to improve code clarity, which aligns with the rules for code quality refactors.
The comment is about a change made in the diff and provides a clear suggestion to improve code clarity by removing a redundant prefix. It should be kept.
Workflow ID: wflow_sf6TI5Db86Dba9K5
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on bdfce1d in 18 seconds
More details
- Looked at
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. .github/workflows/get-logs.yml:46
- Draft comment:
path: ./misc/deploy
- Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The change from 'path' to 'filter' seems intentional, as it is the only change in the diff. The comment suggests reverting this change, but without additional context, it's unclear if 'filter' is incorrect. The comment does not provide strong evidence that 'filter' is wrong, and it could be a valid change.
I might be missing some context about whether 'filter' is a valid option for the 'actions/checkout' action. However, without strong evidence or additional context, I should assume the change is intentional and correct.
The lack of evidence supporting the comment's suggestion to revert the change means it should not be kept. The change could be correct, and the comment does not provide a clear reason to revert it.
Delete the comment because it does not provide strong evidence that the change from 'path' to 'filter' is incorrect.
Workflow ID: wflow_ypHudnslbjPLL4Ox
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 58aa895 in 15 seconds
More details
- Looked at
12lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. .github/workflows/get-logs.yml:46
- Draft comment:
Consider addingpath: misc/deployto thecheckoutaction to align with the PR description. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is suggesting a change that was removed in the diff, which might indicate that the removal was intentional. Without the PR description, it's speculative to assume the intention behind the change. The comment does not provide strong evidence that the change is necessary, and it might be asking to revert an intentional change.
I might be missing the context of the PR description, which could clarify the intention behind the change. However, the rules state not to assume the PR description should be filled out.
Even if the PR description suggests a different intention, the removal of the line in the diff indicates a deliberate change. The comment does not provide strong evidence that the change is required.
Delete the comment as it suggests a speculative change without strong evidence that it is necessary.
Workflow ID: wflow_nSNH6EUnlzBHuGWG
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 8c33420 in 19 seconds
More details
- Looked at
55lines of code in2files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_kKwdQwF0MvX4rG6b
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| @@ -39,4 +40,4 @@ remote_branch_dir="skymp-server-$DEPLOY_BRANCH" | |||
| run_remote test -e "$remote_branch_dir" \ | |||
| || (echo "no branch on remote server" && exit 1) | |||
|
|
|||
| run_remote bash -c "docker logs --tail 100 $remote_branch_dir | grep $GREP_ARG" | |||
| run_remote "bash -c 'docker logs --tail $TAIL_ARG \"$remote_branch_dir\" | grep \"$GREP_ARG\"'" | |||
There was a problem hiding this comment.
Variables should be quoted to handle spaces or special characters correctly.
| run_remote "bash -c 'docker logs --tail $TAIL_ARG \"$remote_branch_dir\" | grep \"$GREP_ARG\"'" | |
| run_remote "bash -c 'docker logs --tail "$TAIL_ARG" "$remote_branch_dir" | grep "$GREP_ARG"'" |
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 7faf0f5 in 10 seconds
More details
- Looked at
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_9HGHKLGmL26QNJsn
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 8cf6e53 in 13 seconds
More details
- Looked at
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. misc/deploy/get_logs_from_server.sh:14
- Draft comment:
The change from${GREP_ARG:?}to${GREP_ARG?}allowsGREP_ARGto be empty, which is acceptable if the script logic can handle it. Ensure that subsequent logic does not fail with an emptyGREP_ARG. - Reason this comment was not posted:
Confidence changes required:50%
The change from${GREP_ARG:?}to${GREP_ARG?}is intentional to allow empty values. This is fine as long as the script logic can handle an emptyGREP_ARG. No issues here.
Workflow ID: wflow_FS0phJbp7bRh8b7N
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on ec51cc9 in 20 seconds
More details
- Looked at
36lines of code in2files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_t3pikcFA6g7QRUOA
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Important
Fix quoting issue in
get_logs_from_server.shand updateget-logs.ymlto handle special characters and addtailinput.get_logs_from_server.shto handle spaces/special characters inremote_branch_dirandGREP_ARG.TAIL_ARGto control log line retrieval inget_logs_from_server.sh.get-logs.ymlto includetailinput parameter with default value10000.checkoutaction to usepath: misc/deploy.This description was created by
for ec51cc9. It will automatically update as commits are pushed.