Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Setting File Access Lists #4369
Changes from 3 commits
d32a28a
a272dbb
ff95cb9
94baebe
97538ff
96ff43a
cfe8cff
a5c6449
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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
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.
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
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 @TheYarinThere 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