-
Notifications
You must be signed in to change notification settings - Fork 122
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
Try to configure authentication for command from GitWithAuth
#884
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #884 +/- ##
============================================
+ Coverage 65.93% 65.98% +0.05%
- Complexity 3376 3379 +3
============================================
Files 360 360
Lines 14019 14025 +6
Branches 1505 1505
============================================
+ Hits 9243 9255 +12
+ Misses 3918 3915 -3
+ Partials 858 855 -3
☔ View full report in Codecov by Sentry. |
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 thought configuring commands right away was more intuitive but this also looks good. 👍
mirrorRemoteToLocal(git, executor, maxNumFiles, maxNumBytes, sessionFactory::configureCommand); | ||
final SshClient sshClient = createSshClient(); | ||
final URIish remoteUri = remoteUri(); | ||
final ClientSession session = createSession(sshClient, remoteUri); |
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.
We need to close the sshClient
even if an exception is raised while creating ClientSession
.
@@ -228,20 +226,18 @@ dirCache, new InsertText(mirrorStatePath.substring(1), // Strip the leading '/'. | |||
} | |||
|
|||
final PushCommand push = git.push(); | |||
configurator.accept(push); | |||
push.setRefSpecs(new RefSpec(headBranchRefName)) |
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.
Could you revert this as well?
git.push().setRefSpecs(...)
GitWithAuth openGit(File workDir, String jGitUri, | ||
URIish remoteUri) throws IOException, URISyntaxException, GitAPIException { |
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 was my mistake. 😓
...mirror-git/src/test/java/com/linecorp/centraldogma/server/internal/mirror/GitMirrorTest.java
Outdated
Show resolved
Hide resolved
final DefaultGitSshdSessionFactory sessionFactory = | ||
new DefaultGitSshdSessionFactory(sshClient, session); | ||
mirrorRemoteToLocal(git, executor, maxNumFiles, maxNumBytes, sessionFactory::configureCommand); | ||
try (GitWithAuth git = openGit(workDir, remoteUri, sessionFactory::configureCommand)) { |
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.
How about using the previous style so that we can use only one try with resources block?
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.
openGit
has a dependency on sessionFactory
. Do you mean initializing the variable as null
and closing in the finally
block manually?
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.
Ah ha, the parameters of openGit
methods are now changed and I forgot about it. 😅
I'm fine with the current approach. 👍
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.
Thanks for cleaning this up! 👍
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.
Thanks, @jrhee17!
Motivation
GitWithAuth
which implies that authentication is done from the object implicitlyConsumer
around for each location where we want to execute a git command