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

fix: chown shenanigans #410

Merged
merged 10 commits into from
Jul 16, 2024
Merged

fix: chown shenanigans #410

merged 10 commits into from
Jul 16, 2024

Conversation

hydazz
Copy link
Member

@hydazz hydazz commented Jul 11, 2024

spawns chown in the background as a 'lazy' way of hopfully negating init stalls
also format scripts and update arm dockerfile

@hydazz
Copy link
Member Author

hydazz commented Jul 11, 2024

also fixing the startup order, should go:
init-check-variables > init-config-immich > init-test-run > init-video-immich > services

@hydazz
Copy link
Member Author

hydazz commented Jul 11, 2024

#394

@ImageGeniusCI
Copy link
Contributor

I am a bot, here are the test results for this PR:
https://ci-tests.imagegenius.io/immich/v1.108.0-pkg-9ec4236b-dev-ed5b28502f05e5ced40020cbc0cb6c208cfe9993-pr-410/index.html
https://ci-tests.imagegenius.io/immich/v1.108.0-pkg-9ec4236b-dev-ed5b28502f05e5ced40020cbc0cb6c208cfe9993-pr-410/shellcheck-result.xml

Tag Passed
amd64-v1.108.0-pkg-9ec4236b-dev-ed5b28502f05e5ced40020cbc0cb6c208cfe9993-pr-410
arm64v8-v1.108.0-pkg-9ec4236b-dev-ed5b28502f05e5ced40020cbc0cb6c208cfe9993-pr-410

@ImageGeniusCI
Copy link
Contributor

I am a bot, here are the test results for this PR:
https://ci-tests.imagegenius.io/immich/v1.108.0-pkg-9ec4236b-dev-370e275ed55b8a230da5cf4f28eb12c2219af456-pr-410/index.html
https://ci-tests.imagegenius.io/immich/v1.108.0-pkg-9ec4236b-dev-370e275ed55b8a230da5cf4f28eb12c2219af456-pr-410/shellcheck-result.xml

Tag Passed
amd64-v1.108.0-pkg-9ec4236b-dev-370e275ed55b8a230da5cf4f28eb12c2219af456-pr-410
arm64v8-v1.108.0-pkg-9ec4236b-dev-370e275ed55b8a230da5cf4f28eb12c2219af456-pr-410

@hydazz hydazz requested a review from martabal July 11, 2024 13:04
Copy link
Collaborator

@martabal martabal left a comment

Choose a reason for hiding this comment

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

Nice

@hydazz
Copy link
Member Author

hydazz commented Jul 11, 2024

@martabal you think running chown in the background is the best way?

@ImageGeniusCI
Copy link
Contributor

I am a bot, here are the test results for this PR:
https://ci-tests.imagegenius.io/immich/v1.108.0-pkg-9ec4236b-dev-898ebc8e245974f7ee394d291978e4965b7b5291-pr-410/index.html
https://ci-tests.imagegenius.io/immich/v1.108.0-pkg-9ec4236b-dev-898ebc8e245974f7ee394d291978e4965b7b5291-pr-410/shellcheck-result.xml

Tag Passed
amd64-v1.108.0-pkg-9ec4236b-dev-898ebc8e245974f7ee394d291978e4965b7b5291-pr-410
arm64v8-v1.108.0-pkg-9ec4236b-dev-898ebc8e245974f7ee394d291978e4965b7b5291-pr-410

@martabal
Copy link
Collaborator

@martabal you think running chown in the background is the best way?

IMO, it is not. Someone who misconfigured his share won't realize this issue and this solution will impact people who don't have permission issues.

@hydazz
Copy link
Member Author

hydazz commented Jul 12, 2024

How about this*

@martabal
Copy link
Collaborator

I think we'll have the same issue #273. We probably want to check if /photos have the correct permissions before changing them

@hydazz
Copy link
Member Author

hydazz commented Jul 12, 2024

this is not a recursive chown, its just the photos folder, and subdirectories 1 depth

@ImageGeniusCI
Copy link
Contributor

I am a bot, here are the test results for this PR:
https://ci-tests.imagegenius.io/immich/v1.108.0-pkg-9ec4236b-dev-054652848f38989e06532134e1f43b1e131a3401-pr-410/index.html
https://ci-tests.imagegenius.io/immich/v1.108.0-pkg-9ec4236b-dev-054652848f38989e06532134e1f43b1e131a3401-pr-410/shellcheck-result.xml

Tag Passed
amd64-v1.108.0-pkg-9ec4236b-dev-054652848f38989e06532134e1f43b1e131a3401-pr-410
arm64v8-v1.108.0-pkg-9ec4236b-dev-054652848f38989e06532134e1f43b1e131a3401-pr-410

@martabal
Copy link
Collaborator

this is not a recursive chown, its just the photos folder, and subdirectories 1 depth

Will this work for files not owned by the abc user (for exemple files in a nfs mounted by root but with Immich started with a different user)?

@hydazz
Copy link
Member Author

hydazz commented Jul 12, 2024

depends on how the nfs permissions are setup, the init scripts are ran as root, so if root has permission to modify the nfs files, then yes, its not being done by abc

cd Immich/
bash: cd: Immich/: Permission denied

whoami
root
cd Immich/

pwd
/mnt/nfs/Picard/Media/Immich

whoami
sandy

long story short, the NFS ACL need to match the UID of the local user set in the PUID

Screenshot 2024-07-12 at 6 14 13 pm

chown is not the way to edit/fix permissions on NFS shares, but we cant check if the mount is nfs soo...

Co-authored-by: martin <74269598+martabal@users.noreply.github.com>
@ImageGeniusCI
Copy link
Contributor

I am a bot, here are the test results for this PR:
https://ci-tests.imagegenius.io/immich/v1.108.0-pkg-9ec4236b-dev-80550b428b5d3ae3a50db47d713ed818c267d9e1-pr-410/index.html
https://ci-tests.imagegenius.io/immich/v1.108.0-pkg-9ec4236b-dev-80550b428b5d3ae3a50db47d713ed818c267d9e1-pr-410/shellcheck-result.xml

Tag Passed
amd64-v1.108.0-pkg-9ec4236b-dev-80550b428b5d3ae3a50db47d713ed818c267d9e1-pr-410
arm64v8-v1.108.0-pkg-9ec4236b-dev-80550b428b5d3ae3a50db47d713ed818c267d9e1-pr-410

@ImageGeniusCI
Copy link
Contributor

I am a bot, here are the test results for this PR:
https://ci-tests.imagegenius.io/immich/v1.108.0-pkg-9ec4236b-dev-9911beacdb02f859b497ea0df493257ed6b0ed2c-pr-410/index.html
https://ci-tests.imagegenius.io/immich/v1.108.0-pkg-9ec4236b-dev-9911beacdb02f859b497ea0df493257ed6b0ed2c-pr-410/shellcheck-result.xml

Tag Passed
amd64-v1.108.0-pkg-9ec4236b-dev-9911beacdb02f859b497ea0df493257ed6b0ed2c-pr-410
arm64v8-v1.108.0-pkg-9ec4236b-dev-9911beacdb02f859b497ea0df493257ed6b0ed2c-pr-410

@hydazz hydazz requested a review from martabal July 13, 2024 06:35
Copy link
Collaborator

@martabal martabal left a comment

Choose a reason for hiding this comment

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

It still does not work ./run: line 7: 8: command not found

@hydazz
Copy link
Member Author

hydazz commented Jul 13, 2024

It still does not work ./run: line 7: 8: command not found

my bad

@hydazz
Copy link
Member Author

hydazz commented Jul 13, 2024

any other changes or good to merge?

@ImageGeniusCI
Copy link
Contributor

I am a bot, here are the test results for this PR:
https://ci-tests.imagegenius.io/immich/v1.108.0-pkg-9ec4236b-dev-80550b428b5d3ae3a50db47d713ed818c267d9e1-pr-410/index.html
https://ci-tests.imagegenius.io/immich/v1.108.0-pkg-9ec4236b-dev-80550b428b5d3ae3a50db47d713ed818c267d9e1-pr-410/shellcheck-result.xml

Tag Passed
amd64-v1.108.0-pkg-9ec4236b-dev-80550b428b5d3ae3a50db47d713ed818c267d9e1-pr-410
arm64v8-v1.108.0-pkg-9ec4236b-dev-80550b428b5d3ae3a50db47d713ed818c267d9e1-pr-410

@martabal
Copy link
Collaborator

Another issue caused during the first install: /usr/bin/find: ‘/photos/*’: No such file or directory

@hydazz hydazz requested a review from martabal July 14, 2024 03:14
find /app/immich -path "*/node_modules" -prune -o -exec chown abc:abc {} +
lsiown -R abc:abc \
/config
[[ "${APPLY_PERMISSIONS}" == "true" ]] && lsiown -R abc:abc "${IMMICH_MEDIA_LOCATION}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm fine with it but maybe we should have it for /config too ?

Copy link
Member Author

Choose a reason for hiding this comment

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

/config should always be mounted locally, and not our problem if mounted any other way IMO

@hydazz hydazz merged commit 7e45f0e into imagegenius:main Jul 16, 2024
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants