This repository was archived by the owner on Sep 30, 2024. It is now read-only.
gitserver: make vcs syncer implementation request a fresh clone URL in between each remote git operation#61179
Merged
Merged
Conversation
3ce14de to
b3867d2
Compare
448d792 to
18c9d65
Compare
ggilmore
commented
Mar 14, 2024
Contributor
Author
There was a problem hiding this comment.
This will be removed soon, this is just a local hack I created to try to revoke tokens without waiting an hour.
a04bf3a to
cea3d55
Compare
pjlast
reviewed
Mar 18, 2024
Contributor
There was a problem hiding this comment.
Whenever I see this []byte -> string -> []byte conversion pattern I wonder if we wouldn't be better off just having []byte implementations of these functions
7270b34 to
999215c
Compare
pjlast
approved these changes
Mar 19, 2024
999215c to
5223749
Compare
mmanela
approved these changes
Mar 19, 2024
ggilmore
commented
Mar 19, 2024
Contributor
Author
There was a problem hiding this comment.
For future readers, this number increased by 3 due to the 3 extra tryWrite calls I added in gitVCSSyncer.Clone (the additional logs before / after SetHEAD + an additional log once the fetch finished): https://github.com/sourcegraph/sourcegraph/pull/61179/files#diff-22d28b810252fd7a9c6e94ce2c5bbbb1a61f079b6a6d8407661f1ae28f282250R111
7348076 to
48c4100
Compare
…URL in between each remote git operation commit-id:4575598b
48c4100 to
c49c84f
Compare
Collaborator
|
The backport to To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-5.3 5.3
# Navigate to the new working tree
cd .worktrees/backport-5.3
# Create a new branch
git switch --create backport-61179-to-5.3
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 cbbb9c3d42a4fa5adad899a0a589d76348dc1df6
# Push it to GitHub
git push --set-upstream origin backport-61179-to-5.3
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-5.3If you encouter conflict, first resolve the conflict and stage all files, then run the commands below: git cherry-pick --continue
# Push it to GitHub
git push --set-upstream origin backport-61179-to-5.3
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-5.3
|
eseliger
reviewed
Mar 27, 2024
Comment on lines
+303
to
+306
| var exitErr *exec.ExitError | ||
| if errors.As(err, &exitErr) { | ||
| // Redact the stderr output if we have it. | ||
| exitErr.Stderr = []byte(redactor(string(exitErr.Stderr))) |
Member
There was a problem hiding this comment.
since we use err not exitErr further down, is exitErr a reference or a copy of err?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
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.
Fixes: https://github.com/sourcegraph/sourcegraph/issues/60682
The key insight into fixing https://github.com/sourcegraph/sourcegraph/issues/60682 is to make our
vcssyncerimplementation (that performs the fetch and clone operations) stop using a "static" remote URL for all of it's operations.Before, when we used a Github App installation, Sourcegraph will receive an authentication token that's valid for one hour. We embed this token in the remote URL that we use to configure git for the fetch and git remote show head (to figure out what the default branch is pointing to) operations. When a huge repository like https://github.com/sgtest/megarepo gets cloned - it's likely that it will take > 1 hour to clone. This means that the auth information embedded the remote URL will expire in between the fetch and remote head operations - which would fail the overall "clone" operation.
The solution to this is to introduce a new abstraction:
The
RemoteURLSourceencapsulates all the logic to figure out what remote URL to use for a given repository (including authentication information, token refreshing, etc.). Instead of passing around a static remote URL, theRemoteURLSourceis stored as a struct member on each of the syncers. Whenever the syncer performs a git operation (like fetch) - it consults the source for the latest remoteURL to use - and only uses that url for the current operation. This means that individual git operations now have the chance to "refresh" the token in the remote URL (if necessary) which solves the original problem.Test plan: