Skip to content
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

Added the missing UMASK section. #288

Merged
merged 1 commit into from
Nov 13, 2024
Merged

Added the missing UMASK section. #288

merged 1 commit into from
Nov 13, 2024

Conversation

uhop
Copy link
Contributor

@uhop uhop commented Aug 20, 2024

See #272

linuxserver.io


  • I have read the contributing guideline and understand that I have made the correct modifications

Description:

Trivial 3-line section to set UMASK to settings.json as per the current documentation.

Benefits of this PR and context:

It fixes the filed bug, which clearly annoys people (including me).

How Has This Been Tested?

I ran it and saw the change in /config/settings.json.

Source / References:

The existing doc:

## Umask for running applications

The bug: #272

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for opening this pull request! Be sure to follow the pull request template!

@LinuxServer-CI
Copy link
Collaborator

I am a bot, here are the test results for this PR:
https://ci-tests.linuxserver.io/lspipepr/transmission/4.0.6-r0-pkg-6730fbea-dev-20cf81ec2e49fec458e140de6b32f0d733094c47-pr-288/index.html
https://ci-tests.linuxserver.io/lspipepr/transmission/4.0.6-r0-pkg-6730fbea-dev-20cf81ec2e49fec458e140de6b32f0d733094c47-pr-288/shellcheck-result.xml

Tag Passed
amd64-4.0.6-r0-pkg-6730fbea-dev-20cf81ec2e49fec458e140de6b32f0d733094c47-pr-288
arm64v8-4.0.6-r0-pkg-6730fbea-dev-20cf81ec2e49fec458e140de6b32f0d733094c47-pr-288

@LinuxServer-CI
Copy link
Collaborator

I am a bot, here are the test results for this PR:
https://ci-tests.linuxserver.io/lspipepr/transmission/4.0.6-r0-pkg-f68ac517-dev-356173a6975a42c4e5ed422e07f0af9baf10e8a6-pr-288/index.html
https://ci-tests.linuxserver.io/lspipepr/transmission/4.0.6-r0-pkg-f68ac517-dev-356173a6975a42c4e5ed422e07f0af9baf10e8a6-pr-288/shellcheck-result.xml

Tag Passed
amd64-4.0.6-r0-pkg-f68ac517-dev-356173a6975a42c4e5ed422e07f0af9baf10e8a6-pr-288
arm64v8-4.0.6-r0-pkg-f68ac517-dev-356173a6975a42c4e5ed422e07f0af9baf10e8a6-pr-288

@LinuxServer-CI
Copy link
Collaborator

I am a bot, here are the test results for this PR:
https://ci-tests.linuxserver.io/lspipepr/transmission/4.0.6-r0-pkg-2cb64b31-dev-f5cd1a2f5fbf0f94c760cef575dbcb3e1410e1fb-pr-288/index.html
https://ci-tests.linuxserver.io/lspipepr/transmission/4.0.6-r0-pkg-2cb64b31-dev-f5cd1a2f5fbf0f94c760cef575dbcb3e1410e1fb-pr-288/shellcheck-result.xml

Tag Passed
amd64-4.0.6-r0-pkg-2cb64b31-dev-f5cd1a2f5fbf0f94c760cef575dbcb3e1410e1fb-pr-288
arm64v8-4.0.6-r0-pkg-2cb64b31-dev-f5cd1a2f5fbf0f94c760cef575dbcb3e1410e1fb-pr-288

Copy link
Member

@drizuid drizuid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

im tentatively on board with this fix, i forgot about this issue, tbh. Howeever, i have some concerns about weird shit qnap/synology/unraid might do in terms of umask.. i don't think it should cause a problem, but I will ping the rest of the team to give thoughts before merge.

@drizuid drizuid requested a review from a team October 16, 2024 12:48
@drizuid drizuid added the work-in-progress Stale exempt label Oct 16, 2024
@LinuxServer-CI
Copy link
Collaborator

I am a bot, here are the test results for this PR:
https://ci-tests.linuxserver.io/lspipepr/transmission/4.0.6-r0-pkg-90692212-dev-e5db864c37fe096657d67a817ecedf76bc1e638a-pr-288/index.html
https://ci-tests.linuxserver.io/lspipepr/transmission/4.0.6-r0-pkg-90692212-dev-e5db864c37fe096657d67a817ecedf76bc1e638a-pr-288/shellcheck-result.xml

Tag Passed
amd64-4.0.6-r0-pkg-90692212-dev-e5db864c37fe096657d67a817ecedf76bc1e638a-pr-288
arm64v8-4.0.6-r0-pkg-90692212-dev-e5db864c37fe096657d67a817ecedf76bc1e638a-pr-288

@zwimer
Copy link

zwimer commented Oct 31, 2024

Thanks for addressing this issue, I currently run a script that periodically chmod's stuff so I'm glad I'll soon be able to get rid of that :)

@positiveque
Copy link

Thanks for addressing this issue, I currently run a script that periodically chmod's stuff so I'm glad I'll soon be able to get rid of that :)

Could you share your script? Thanks.

@nemchik
Copy link
Member

nemchik commented Nov 11, 2024

My only thought here is I would recommend doing it via jq rather than sed in case anything about the settings.json changes in a way that breaks the sed pattern. jq should be more resilient in that regard.

@uhop
Copy link
Contributor Author

uhop commented Nov 12, 2024

My only thought here is I would recommend doing it via jq rather than sed in case anything about the settings.json changes in a way that breaks the sed pattern. jq should be more resilient in that regard.

Sounds like a good idea, but take a look at the modified file: https://github.com/linuxserver/docker-transmission/blob/20cf81ec2e49fec458e140de6b32f0d733094c47/root/etc/s6-overlay/s6-rc.d/init-transmission-config/run

It uses sed 16 times. I added the change (one more use of sed) in the same style to be consistent, to reduce possible dependencies and to lessen the cognitive load on future maintainers.

If you want to rewrite the whole file (and other files?) — please let me know. I would recommend retooling and reimplementation as a separate PR because it is the orthogonal thing not directly related to the bug.

@nemchik
Copy link
Member

nemchik commented Nov 13, 2024

My only thought here is I would recommend doing it via jq rather than sed in case anything about the settings.json changes in a way that breaks the sed pattern. jq should be more resilient in that regard.

Sounds like a good idea, but take a look at the modified file: https://github.com/linuxserver/docker-transmission/blob/20cf81ec2e49fec458e140de6b32f0d733094c47/root/etc/s6-overlay/s6-rc.d/init-transmission-config/run

It uses sed 16 times. I added the change (one more use of sed) in the same style to be consistent, to reduce possible dependencies and to lessen the cognitive load on future maintainers.

If you want to rewrite the whole file (and other files?) — please let me know. I would recommend retooling and reimplementation as a separate PR because it is the orthogonal thing not directly related to the bug.

Fair point about the 16 uses of sed. I actually think it makes more sense to replace them all with jq. We include it as a dependency in the base image:
https://github.com/linuxserver/docker-baseimage-alpine/blob/4356a5ee099f5286605bebc1b379b99c35cf4c75/Dockerfile#L82

I believe the use of sed predate jq's addition to the base image. My reasoning for recommending jq is it should handle parsing the json file even if formatting changes (mainly whitespace, likely accidentally). Also with jq the logic would be to add the value, and since jq will parse the json when doing this it would replace existing values or add new ones, which in my view is more reliable, and if parsing fails it would reveal issues during init more consistently.

With all that said, looking at the run file, we also have a lot of things like:

if [[ condition ]]; then
    do A
    do B
else
    do C
    do B
fi

Where B is done regardless of the condition, so that could certainly be cleaned up and refactored.

jq has some quirks, such as not being able to write to the same file it reads from, but this is easily overcome with some bash shenanigans. It also has a fairly safe way to interpolate variable values into the json. This is the syntax I would recommend:

echo -E "$(jq -r --arg umask "${UMASK}" '.umask = $umask' settings.json)" > settings.json

Unfortunately all of this will make your PR more complex. With that said, and now that I've typed it all out, I think accepting this PR as is makes the most sense, and opening a second PR to refactor the run file after this merges would be fun but not required.

@drizuid drizuid merged commit 409c4e2 into linuxserver:master Nov 13, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
work-in-progress Stale exempt
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants