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

Remove windows warning about running containers as root, fixes #800 #803

Merged
merged 3 commits into from
May 9, 2018

Conversation

rfay
Copy link
Member

@rfay rfay commented Apr 20, 2018

The Problem/Issue/Bug:

OP #800 - Awkward warning (repeated lots) about Windows containers running as root.

This is not a threat on Docker for Windows, and in fact doesn't seem to work otherwise, so omit the warning for that situation.

Manual Testing Instructions:

Run on windows, verify correct behavior:

  • Should be able to create upload files in sites/default/files
  • Files created should be owned by the host user
  • The warning should be gone.

It's reasonable to do modest test on another environment too, maybe linux with a UID like 65000.

Note that the latest push of this has the windows installer, so should be pretty easy to do a runthrough.

Automated Testing Overview:

  • Added TestWriteableFilesDirectory(), which should detect if we have the returning trouble with not being able to write files to the upload directory.

Related Issue Link(s):

OP #800

Release/Deployment notes:

@rfay rfay requested a review from rickmanelius April 20, 2018 22:49
@rfay rfay changed the title Run containers as UID 1000 on Windows Run containers as UID 1000 on Windows, fixes #800 Apr 20, 2018
@rfay rfay self-assigned this Apr 20, 2018
@rickmanelius rickmanelius requested a review from cweagans May 1, 2018 18:11
@rickmanelius
Copy link
Contributor

I think @cweagans will be able to test this more quickly than me. I'm still slow on Windows because I don't spend a lot of time using Parallels except when absolutely necessary. Adding him to the reviewer list.

@rfay
Copy link
Member Author

rfay commented May 1, 2018

This is WIP, not ready for testing.

@cweagans
Copy link
Contributor

cweagans commented May 1, 2018

I don't have a functioning Windows install at the moment, but it's on my list for the weekend. I can do it sooner if necessary though.

@rfay rfay force-pushed the 20180420_run_windows_as_1000 branch from 7691782 to ce614d5 Compare May 2, 2018 18:19
@rfay rfay changed the title Run containers as UID 1000 on Windows, fixes #800 Remove windows warning about running containers as root, fixes #800 May 2, 2018
@rfay
Copy link
Member Author

rfay commented May 3, 2018

This is ready for review; I went another way, and added a test for writeability of files directory.

@rfay rfay force-pushed the 20180420_run_windows_as_1000 branch 3 times, most recently from ab14415 to 1bb1e2c Compare May 4, 2018 13:43
@rfay rfay force-pushed the 20180420_run_windows_as_1000 branch from 1bb1e2c to 6613afc Compare May 4, 2018 19:51
@rfay
Copy link
Member Author

rfay commented May 4, 2018

Note that the latest push of this has the windows installer, so should be pretty easy to do a runthrough.

@rfay rfay mentioned this pull request May 8, 2018
6 tasks
Copy link
Contributor

@tannerjfco tannerjfco left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@rfay rfay merged commit 90cb194 into ddev:master May 9, 2018
@rfay rfay deleted the 20180420_run_windows_as_1000 branch May 9, 2018 15:10
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.

None yet

4 participants