build: Add a git safe.directory #4830
Conversation
Call that command from the postCreateCommand in devcontainer.json
richb-hanover
changed the title
|
Great, thanks @richb-hanover ! |
| @@ -382,6 +382,9 @@ tasks: | |||
| dir: prqlc/bindings/php | |||
| cmds: | |||
| - vendor/bin/phpunit tests | |||
|
|
|||
| add-safe-directory: | |||
| - git config --global --add safe.directory /workspaces/prql | |||
There was a problem hiding this comment.
I do not think this should be included in the Taskfile. There are very limited situations where this absolute path is valid.
There was a problem hiding this comment.
I see the difficulty. /workspaces/prql works fine for the Dev Container, but is unlikely to be valid in most other situations.
How could this be modified?
(I suspect this bites 100% of people who work on PRQL. For those who compile natively, it's a one-time fix; for Dev Container folks (like me) I have to re-run this command each time the container gets rebuilt...)
There was a problem hiding this comment.
I tend to agree with the downside, but where should it go?
(and do you happen to know why we get this problem?)
There was a problem hiding this comment.
I have not seen this problem occur, and if it does occur, it is most likely a problem with the devcontainer CLI and should not be addressed in this repository.
I suggest reverting this change.
There was a problem hiding this comment.
(and do you happen to know why we get this problem?)
Maybe this issue microsoft/vscode-remote-release#8656
There was a problem hiding this comment.
I am quite skeptical about the conditions under which this problem arises in the first place.
You see, I have been using Dev Containers for years and have never encountered this problem (of course, it was only relatively recently, a few years ago, when Git security was changed and this kind of error started occurring).
But my suspicion is that it is a user manipulation issue and that setting this up for all containers is the wrong direction.
At the very least, we should be very aware that this is an issue that should be resolved upstream and reported upstream.
There was a problem hiding this comment.
We're not the only repo which has this problem — my link above was: https://github.com/codespell-project/codespell/pull/3361/files...
I agree with the principle of solving the root issue rather than adding our patches — but adding a small patch with a TODO: not sure why we need this, but we get error $foo without it close to the issue seems like a reasonable balance, no?
There was a problem hiding this comment.
Sorry, I did check and it seems that even moby is doing that....... (moby/moby#44930)
https://github.com/moby/moby/blob/5efbb60a4efa327301066fa4960b25d8917bf776/Dockerfile#L516-L518
but adding a small patch with a
TODO: not sure why we need this, but we get error $foo without itclose to the issue seems like a reasonable balance, no?
Of course, if this issue has been reported upstream and it is clear that this is the appropriate workaround for the current situation, then I think it is appropriate to make this change to this repository.
There was a problem hiding this comment.
OK, let's wait to hear from the upstream issue then. Thanks.
There was a problem hiding this comment.
Let's move this conversation to #4836 (comment) which has much more context
|
@max-sixty @eitsupi - I didn't mention you in my previous response, and wasn't sure if you would be notified... |
This PR was notified while I was out, but was merged before I got home, thus the delay in commenting. |
Add the github command in the Taskfile.yaml to add a git safe.directory;
Call that command from the postCreateCommand in devcontainer.json;
Fixes #4817