-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Setting File Access Lists #4369
Conversation
@ucheNkadiCode We will need to add a redirect on the website for this file rename so that people who already have bookmarked the old URL will go to the new URL. I can add the redirect in the private website repo before this PR is merged. |
@ucheNkadiCode @ghogen I'll do the merge once everyone is signed off so I can control the timing relative to the website redirect. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ucheNkadiCode We will need to add a redirect on the website for this file rename so that people who already have bookmarked the old URL will go to the new URL. I can add the redirect in the private website repo before this PR is merged.
Per the DateApproved, yes, it is good to update the date so users know the topic is getting attention and is not stale.
Thank you @gregvanl! I have updated the DateApproved. I appreciate the help with the website redirect! We are gearing up to have our aka.ms links point to the new link whenever that goes live. Please either you or @ghogen send me the exact link so I don't mess it up!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving with a few small suggestions.
Co-authored-by: Gordon Hogenson <[email protected]>
Co-authored-by: Gordon Hogenson <[email protected]>
Co-authored-by: Gordon Hogenson <[email protected]>
@ucheNkadiCode The update has been published. |
|
||
1. From the container command line, run one of these commands: | ||
|
||
```powershell |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why powershell and not shell/bash?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same for line 173
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's just telling the docs site how to syntax highlight this chunk of code. Those commands are actually shell/bash commands.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I know, this might seem a little petty but why not mark the block specifically as bash
/sh
? I'm pretty sure whichever markdown engine is used, it supports this language.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... or shell
I agree with @TheYarin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @TheYarin! I'll add this change to the docs
|
||
In order to give access to a non-root user `appuser` from within the container, follow these steps: | ||
|
||
1. From the container command line, run one of these commands: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it's worth mentioning how to enter a container's commandline
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bwateratmsft or @TheYarin Would it be sufficient to say
- From the Containers pane in the Docker View, right click on your running container, select Attach Shell, and run one of these commands:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, I think the user will be doing all this permissions configuration in a production environment, so docker actions from within the developer's VSCode might not be relevant.
(Unless managing your remote docker host from within VSCode is really easy, of which I might not be familiar with)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TheYarin, so you're expecting this scenario will only/mostly occur as a remote container and not a local container on the user's PC? If the user is ever in the situation when it's local, then the steps above work. Otherwise, we have a remote containers extension that the user could use for SSH
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'm expecting this scenario will mostly occur as a remote container. This might happen in the development environment if it's a linux environment (when mounting a volume on my Windows dev environment it didn't cause any permissions errors), but honestly, I got a feeling that a lot of developers don't properly test their application in a containerized setup on their dev machine, because the developer probably has all the dependencies set up already so it's easier for him to just run the app outside the container.
But considering what @karolz-ms said below about the best practice regarding UID, I think mentioning how to enter a container's commandline is kind of irrelevant.
|
||
In order to give access to a non-root user `appuser` from within the container, follow these steps: | ||
|
||
1. From the container command line, run one of these commands: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure the user/group ID will always be the same if the container is removed and re-created? That's a common scenario when operating with docker-compose up
& docker-compose down
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@karolz-ms or @bwateratmsft, do you know about this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know for sure, but I would think that the uid/gid would stay the same, since it comes from the image, not the container. I think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch @TheYarin Best practice is to use and explicit UID in this case: https://docs.docker.com/develop/develop-images/dockerfile_best-practices/#user
Responded to user request for more information on how to add file permissions to access files on a host machine from within a container. I went ahead and revamped the document.
Important: I chose to change the title, so I have also since renamed the root file. This means that the URL will change. I need to know AS SOON AS the link is active because this will affect our dockerfile scaffolding microsoft/vscode-docker#2747.
Question: Should the DateApproved metadata be updated to indicate when the doc was last edited? Or should we keep it as April 2020?
Thank you!