Add support for git includIf config#755
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR threads a workingDir through git configuration resolution: GetUser and ExtractGitConfiguration accept a workingDir and run git config in that directory when provided so includeIf "gitdir:..." directives resolve relative to the project. Callers derive the directory from workspace metadata or pass empty string. ChangesGit includeIf resolution with directory context
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
bce5bf0 to
9b27e74
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/devcontainer/setup/setup.go`:
- Line 467: The call to gitcredentials.GetUser(userName, "") disables
directory-aware git resolution; change the second argument from "" to the
workspace working directory (the variable representing the repo/workspace path
in this scope—e.g., workspaceDir or workDir) so GetUser can resolve includeIf
and repo-specific identities correctly; if that path is not already available,
compute/obtain the absolute workspace directory and pass it into
gitcredentials.GetUser(userName, workspaceDir).
In `@pkg/gitsshsigning/utils.go`:
- Around line 32-36: The readGitConfigValue function currently runs "git config
--get <key>" and may read repo-local config from the current process directory;
change it so when workingDir == "" the git command includes the "--global" flag
to force reading the user's global ~/.gitconfig; update the logic around
exec.Command/cmd.Args in readGitConfigValue to insert "--global" before "--get"
when no workingDir is provided so the call becomes "git config --global --get
<key>" in that case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 622e9537-e9b6-45c8-a9fc-3930bdfc16e4
📒 Files selected for processing (9)
cmd/agent/container/credentials_server.gopkg/agent/tunnelserver/tunnelserver.gopkg/devcontainer/setup/setup.gopkg/gitcredentials/gitcredentials.gopkg/gitcredentials/gitcredentials_test.gopkg/gitsshsigning/utils.gopkg/gitsshsigning/utils_test.gopkg/tunnel/services.gopkg/tunnel/services_test.go
skevetter
left a comment
There was a problem hiding this comment.
This may be complex, but it would be helpful if you included an integration test of this new functionality in e2e/tests
ce06447 to
c6edf63
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/devcontainer/setup/setup.go`:
- Line 163: The code is passing
cfg.SetupInfo.SubstitutionContext.LocalWorkspaceFolder into
gitcredentials.GetUser which can fail inside a container because the host path
may not exist; update the call site to pass the container path instead
(cfg.SetupInfo.SubstitutionContext.ContainerWorkspaceFolder) when available,
falling back to LocalWorkspaceFolder only if ContainerWorkspaceFolder is empty,
and ensure the change is applied where gitcredentials.GetUser is invoked
(reference the GetUser call and
cfg.SetupInfo.SubstitutionContext.LocalWorkspaceFolder/ContainerWorkspaceFolder
symbols).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8b125a7f-ee12-4a00-b2aa-0c6eede4b51e
📒 Files selected for processing (10)
cmd/agent/container/credentials_server.goe2e/tests/ssh/ssh.gopkg/agent/tunnelserver/tunnelserver.gopkg/devcontainer/setup/setup.gopkg/gitcredentials/gitcredentials.gopkg/gitcredentials/gitcredentials_test.gopkg/gitsshsigning/utils.gopkg/gitsshsigning/utils_test.gopkg/tunnel/services.gopkg/tunnel/services_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- pkg/gitsshsigning/utils.go
- cmd/agent/container/credentials_server.go
- pkg/gitcredentials/gitcredentials.go
- pkg/gitsshsigning/utils_test.go
- pkg/gitcredentials/gitcredentials_test.go
- pkg/agent/tunnelserver/tunnelserver.go
- pkg/tunnel/services.go
c6edf63 to
5b23d7a
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
f65615a to
a883d4f
Compare
|
FYSA, you can add additional commits (e.g., not necessary to continue force pushing). The commits are squashed before they are merged to main. |
6680ed1 to
b5d247c
Compare
509af3c to
eb42ffc
Compare
I made a few tries but couldn't make an e2e test that confirms this works. I built and used devpod with this feature for past week and it's working for my use case at least. I can cleanup the debug logs I added and either keep the e2e test or remove it. if you have ideas on how to test includeIf too let me know I can give it another try :) |
I haven't investigated this, but it looks like the jobs are passing with your most recent changes except for lint. As long as this isn't breaking the SSH tunnel (I don't think it is), I am fine with dropping the e2e test/debug logs to ship the change. If you want, you can add the integration tests as a follow-up. Not a requirement from me. |
eb42ffc to
11c2d72
Compare
The includeIf test shows user.email never being set despite the credentials server completing successfully. Add debug logs to the host-side GitUser RPC handler (logging workingDir and GIT_CONFIG_GLOBAL) and to the container-side configureGitUserLocally (logging what the host returned). Also add --debug to the e2e test's devpod ssh call and dump ~/.gitconfig + GIT_CONFIG* env on timeout so the next CI run reveals the exact failure point. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
11c2d72 to
fbd1529
Compare
|
great, thx. I cleaned up what I added to try to debug the e2e test, build it again and tried it on my project to confirm I didn't break anything during my experiements it's all green on my side :) |
skevetter
left a comment
There was a problem hiding this comment.
Great work! Thanks for adding this!
closes #750
Summary by CodeRabbit
Bug Fixes
Tests