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

feat: check fs perms on startup #530

Closed
wants to merge 1 commit into from

Conversation

focusaurus
Copy link
Contributor

@focusaurus focusaurus commented May 31, 2019

We had a user report filesystem errors launching storefront. I'm adding this script so we can detect this on startup time and print a clear error message with the commands necessary to fix this.

Note that I'm proposing the following solution to the tricky problem of userid mismatch between the container and the host OS. If anyone has a better suggestion, I'm all ears. We recommend changing the group of files to match what we use in the container (1000) and allowing group read/write/execute (for directories). I think this will allow harmonious use in both the host OS as the user's normal user and in the container as the node user.

Testing

  • Use chown, chgrp, and chmod to configure various permutations of owner/group/read/write/execute permissions on "${HOME}/.cache/yarn-offline-mirror"
    • You may also want to test permutations of .cache not existing at all, yarn-offline-mirror not existing, etc
  • Run ./bin/check-fs-perms.sh from both the host OS and from within the running container
  • Verify you get errors and a good error message when perms are wrong
  • Verify you get a zero exit code and just a 1-line status indication when perms are OK
  • Verify starterkit fails at startup with a clear error message when perms are wrong
  • Verify starterkit starts properly when perms are OK, and no noticeable amount of time is added to startup time
  • Verify a straight copy/paste of the commands in the error message to a host shell properly resolves the problem
  • Test on both linux and mac

Further instances

If we are happy with this solution, I will recommend we make similar changes to other projects that use docker volume mounts from the host filesystem. This includes at least reaction core, maybe other projects as well.

If yarn-offline-mirror isn't rwx, exit and print good error message

Signed-off-by: Peter Lyons <[email protected]>
@focusaurus focusaurus self-assigned this May 31, 2019
@focusaurus
Copy link
Contributor Author

Copy link
Member

@ticean ticean left a comment

Choose a reason for hiding this comment

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

I think there may be a better way to do this? I typed up a full description in reactioncommerce/reaction-development-platform#14 (comment).

I think we can solve this with and ENV in Docker Compose and that solution would not require any scripting.

Could we review that or pair before merging this? I'd like to fully exhaust the existing user mapping options avialable in Docker and Compose before adding scripting as a solution.

@focusaurus
Copy link
Contributor Author

Yeah, if we can get HOST_UID working I think that's a better solution. The caveat is that will probably generate an avalanche of transitional errors in the existing install base and we'll probably want some scripting to fix things up if folks have root-owned files or 1000-owned files from the current approach.

@focusaurus
Copy link
Contributor Author

I'm going to close this and see if we can re-activate the other solution that has been stalled.

@focusaurus focusaurus closed this May 31, 2019
@rosshadden rosshadden deleted the feat-pete-check-fs-permission branch May 31, 2019 18:51
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.

2 participants