-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[supervisor] Don't install git msg annotation hook in headless workspaces #20548
Conversation
Commits still seem to be annotated |
@@ -211,6 +221,14 @@ func Run(options ...RunOption) { | |||
symlinkBinaries(cfg) | |||
|
|||
configureGit(cfg) | |||
go func() { |
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.
@filiptronicek What about moving the go fun()...
into `configureGit()´? Feels that would match the pattern we have going here, and reduces the noise on the outside. 👍
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.
Sounds good, will move it to reduce the LOC we have in the main supervisor func. The reason I put it here at first is to not have to pass the cstate around.
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.
Fixed in 0712aea
(#20548)
@@ -845,15 +846,21 @@ func setupGitMessageHook(path string) error { | |||
if err := os.MkdirAll(path, 0755); err != nil { | |||
return err | |||
} | |||
if err := os.Chown(path, gitpodUID, gitpodGID); err != nil { |
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.
Hm, wonder whether there is a nicer way to do that... but can't imagine one, besides resorting to a shell and exec.Command
etc.
So I guess what we have here is the best solution. 👍
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.
Looks like Claude agrees with me, but it offered to move the chown block so that the two chowns are just besides each other. Made that change in c0de48f
(#20548), but if we ever introduce more hooks, we could recursively walk the tree to chown instead of calling os.Chown
multiple times explicitly.
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.
Description
Fixes an issue with setting up the supervisor git hook we've seen in catfood, during content init when restoring the prebuild snapshot,
clearWorkspace
doesn't have enough permissions to clean everything in the git repo:gitpod/components/content-service/pkg/initializer/prebuild.go
Lines 63 to 89 in 087c38e
This is fixed by:
gitpod
userContentReady
How to test
https://github.com/gitpod-io/gitpod/pull/20545
as context)