-
Notifications
You must be signed in to change notification settings - Fork 23
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
Use Agent for parsing Git repo names (CODY-3132) #2257
Conversation
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.
Good intuition to de-dup something. In particular, the Kotlin version of this function doesn't handle server-side repo renaming.
Unfortunately the implementation doesn't look right--see the comment inline for the approach we need here.
@@ -128,7 +132,25 @@ private static String doReplacements( | |||
|
|||
if (vcsType == VCSType.GIT && repository != null) { | |||
String cloneURL = GitUtil.getRemoteRepoUrl((GitRepository) repository, project); | |||
return convertGitCloneURLToCodebaseNameOrError(cloneURL).getValue(); | |||
AtomicReference<String> repoURl = new AtomicReference<>(); |
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.
This isn't right. withAgent
runs the callback on a separate thread, so you will often--but not always--have null in repoURl
.
You need to:
- Make
getRemoteRepoUrl
return a CompletableFuture<String?> - Convert the callers to handle an asynchronous result. This may take care and attention to not block EDT, not starve a threadpool, ensure that if you started on EDT you continue on EDT if the caller requires EDT, etc.
- Convert the method body to handle the asynchronous result: Create the future, run the code with Agent, complete the result in the callback.
By the way, let's pay attention to details like repoURL
or repoUrl
but not repoURl
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 for some reason I thought it was blocking. I've updated it in this commit. Do you know if there are any unit tests of agent integration? Would this be a good use case for an integration test?
} | ||
|
||
// Handle Azure DevOps URLs | ||
if (uri.host?.contains("dev.azure") == true && !uri.path.isNullOrEmpty()) { |
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.
Are all of these cases handled in unit tests on the Agent side?
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.
Yes in this file. In fact it looks like the tests here were carbon copied from there.
This function was in agent long time ago, and was copied to jetbrains because it turned out to be problematic. Also actions like |
@pkukielka @dominiccooney agreed on the testing required. Do either of you have a suggestion of how to test this? Do we have any tests for agent communication? Should this be an integration test? |
I removed part of the code which was not needed and fixed the duplicated code. |
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.
The option - Enable Cody
- decides whether the agent runs or not.
Users can have Enable Cody
turned off and still want to use Sourcegraph Actions.
Closing this as:
|
CODY-3132
There was a duplication of effort around the convertGitURLToCodebaseName as it was implemented in the agent and here. There was even already an agent method for this, but it was unused. Thus when fixing the above on the agent side, I thought it made sense to switch it here.
Test plan
I'm not sure the best way to test this, so input would be appreciated! Do we have tests for agent integration?