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 permissions issue caused by a8a8b8f #1125

Merged
merged 8 commits into from May 15, 2024
Merged

Conversation

shitwolfymakes
Copy link
Member

@shitwolfymakes shitwolfymakes commented May 10, 2024

Description

Fixed an issue created by removing permissions setting in commit a8a8b8f. Confirmed working after running the rebuilt container.

The wiki now includes a note about default permissions for newly created folders and how that make affects users on mounting file shares

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce.
Please also list any relevant details for your test configuration

  • Docker

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have tested that my fix is effective or that my feature works

@shitwolfymakes shitwolfymakes self-assigned this May 10, 2024
@shitwolfymakes shitwolfymakes added Docker Issues with the docker version Bugfix Bugfixes in PR labels May 10, 2024
@shitwolfymakes
Copy link
Member Author

@SylvainMT This will fix your issue once merged and released

Copy link
Collaborator

@microtechno9000 microtechno9000 left a comment

Choose a reason for hiding this comment

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

Agree with the fix, seems the removal of chown to resolve a specific issue was not worth the general hassle.

Can you amend the wiki to explain that the default behaviour sets the user ARM as the owner using chown.

There are users that have mounted the ARM folders via a samba or NFS share that this will cause issues. Need to make sure those users are aware and mounting network shares is not the preferred config.

@shitwolfymakes
Copy link
Member Author

I see what you mean now. The way this is fixing the permissions issue will break permissions for shares every time it boots. I'll figure out a different way to fix this.

shitwolfymakes and others added 4 commits May 15, 2024 07:14
Without this change, every time a user runs the container it will reset the permissions of the folders. Otherwise this could break ownership of arm folders mounted from SMB/NFS shares every time the container is run.
Copy link

sonarcloud bot commented May 15, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@shitwolfymakes
Copy link
Member Author

@SylvainMT This will no longer automatically fix your issue. If you are not using an external share for the container, running this should fix the permissions issues:

sudo chown -R arm:arm /home/arm

Copy link
Collaborator

@microtechno9000 microtechno9000 left a comment

Choose a reason for hiding this comment

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

good with the proposed changes, best of both worlds

@microtechno9000 microtechno9000 merged commit 2059beb into main May 15, 2024
12 checks passed
@SylvainMT
Copy link
Contributor

I also agree, that permissions need only be set at folder creation, not every time the container starts. Thanks for looking into the issue :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bugfix Bugfixes in PR Docker Issues with the docker version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants