-
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
main: maintain runner perms when mounting a stickydisk #5
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 1254973 in 19 seconds
More details
- Looked at
53
lines of code in1
files - Skipped
1
files when reviewing. - Skipped posting
5
drafted comments based on config settings.
1. src/main.ts:36
- Draft comment:
Usingsudo
in shell commands can be a security risk. Ensure that the commands are safe and that the user running the script has the necessary permissions. - Reason this comment was not posted:
Confidence changes required:50%
The use ofsudo
in shell commands can be a security risk if not handled properly. It's important to ensure that the commands are safe and that the user running the script has the necessary permissions.
2. src/main.ts:42
- Draft comment:
Usingsudo
in shell commands can be a security risk. Ensure that the commands are safe and that the user running the script has the necessary permissions. - Reason this comment was not posted:
Confidence changes required:50%
The use ofsudo
in shell commands can be a security risk if not handled properly. It's important to ensure that the commands are safe and that the user running the script has the necessary permissions.
3. src/main.ts:62
- Draft comment:
Usingsudo
in shell commands can be a security risk. Ensure that the commands are safe and that the user running the script has the necessary permissions. - Reason this comment was not posted:
Confidence changes required:50%
The use ofsudo
in shell commands can be a security risk if not handled properly. It's important to ensure that the commands are safe and that the user running the script has the necessary permissions.
4. src/main.ts:85
- Draft comment:
Usingsudo
in shell commands can be a security risk. Ensure that the commands are safe and that the user running the script has the necessary permissions. - Reason this comment was not posted:
Confidence changes required:50%
The use ofsudo
in shell commands can be a security risk if not handled properly. It's important to ensure that the commands are safe and that the user running the script has the necessary permissions.
5. src/main.ts:88
- Draft comment:
Usingsudo
in shell commands can be a security risk. Ensure that the commands are safe and that the user running the script has the necessary permissions. - Reason this comment was not posted:
Confidence changes required:50%
The use ofsudo
in shell commands can be a security risk if not handled properly. It's important to ensure that the commands are safe and that the user running the script has the necessary permissions.
Workflow ID: wflow_EOFBola1B2wc2XiC
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
1254973
to
229eaba
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 229eaba in 19 seconds
More details
- Looked at
83
lines of code in2
files - Skipped
1
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. src/main.ts:36
- Draft comment:
Ensure that the use ofsudo
is necessary for this command. Whileblkid
typically requires root privileges, consider if there are any alternatives or configurations that could avoid the need forsudo
. This comment applies to other uses ofsudo
in this file as well. - Reason this comment was not posted:
Confidence changes required:50%
The use ofsudo
in theexecAsync
calls is necessary for operations that require root privileges, such as formatting and mounting block devices. However, the use ofsudo
should be minimized to only those commands that absolutely require it, to reduce potential security risks.
2. .github/workflows/bump-tag.yaml:22
- Draft comment:
Using-f
withgit tag
andgit push
can overwrite existing tags. Ensure this is the intended behavior, as it can lead to loss of tag history. - Reason this comment was not posted:
Confidence changes required:50%
Thebump-tag.yaml
workflow file is responsible for updating a Git tag. The use of-f
ingit tag
andgit push
is forceful and should be used with caution. It can overwrite existing tags, which might not be the intended behavior.
Workflow ID: wflow_iyYD9vHupWS9J9qJ
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
229eaba
to
d29289d
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 d29289d in 31 seconds
More details
- Looked at
82
lines of code in2
files - Skipped
1
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. .github/workflows/bump-tag.yaml:4
- Draft comment:
Consider addingpull_request:
to theon
section to trigger the workflow on pull request events as well, as mentioned in the PR description. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
This workflow's purpose is to manually update a git tag. It would not make sense to run this on pull requests since: 1) We don't want to automatically bump tags on PRs 2) The workflow requires write permissions to push tags 3) The workflow explicitly checks out main branch 4) The comment assumes something about PR description which violates our rules.
Maybe there's a valid use case for running this on PRs that I'm not seeing? The PR description might have important context I'm missing.
No - this is a clear case where running on PRs would be incorrect. The workflow is designed for manual tag management and running it on PRs could cause tag conflicts and unintended side effects.
Delete this comment. It suggests a change that would be harmful to the workflow's purpose, and it also violates our rule about not making assumptions based on PR descriptions.
Workflow ID: wflow_SC6oDHEtEH2rEB74
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Important
Maintain runner permissions when mounting stickydisk by setting filesystem and mount point ownership, and add GitHub Actions workflow for tag management.
maybeFormatBlockDevice()
, usesudo mkfs.ext4
with-E root_owner=$(id -u):$(id -g)
to set filesystem ownership to runner user.resize2fs
operation preserves existing filesystem ownership and permissions.mountStickyDisk()
, create mount point withoutsudo
to ensure runner user ownership.sudo chown $(id -u):$(id -g)
after mounting to set mount point ownership to runner user.bump-tag.yaml
workflow to bumpv1
tag onworkflow_dispatch
andpull_request
events.This description was created by for d29289d. It will automatically update as commits are pushed.