-
Notifications
You must be signed in to change notification settings - Fork 355
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
Add support for external auth providers in code search #6919
base: main
Are you sure you want to change the base?
Conversation
dfc3ef0
to
aa904c1
Compare
aa904c1
to
4a47767
Compare
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.
code looks good. I am going to test it
@@ -343,7 +343,9 @@ tasks { | |||
return destinationDir | |||
} | |||
|
|||
val sourcegraphDir = unzipCodeSearch() | |||
val codeSearchDirOverride = System.getenv("CODE_SEARCH_DIR_OVERRIDE") |
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.
nice
} | ||
} | ||
return {} |
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.
previously we had a console.error
. do we need it? or is {}
a valid option?
@@ -19,8 +19,6 @@ class CodySettingChangeListener(project: Project) : ChangeListener(project) { | |||
CodySettingChangeActionNotifier.TOPIC, | |||
object : CodySettingChangeActionNotifier { | |||
override fun afterAction(context: CodySettingChangeContext) { | |||
// Notify JCEF about the config changes |
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.
don't we need to refreshConfiguration
now?
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 config was already not containing any info useful for sourcegraph search.
We care about endpoint and headers.
Endpoint change is signalled in CodyAuthService
.
Headers are fetched every time so we do not need to notify about any change in their case.
} catch (Exception ignored) { | ||
return false; | ||
} |
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.
is it safe to ignore it and return false? does it make sense to log/warn here?
Changes
This PR adds agent endpoint which allow us to fetch auth headers, which in turn we need to deliver to the sourcegraph search webview.
Webview changes are on a separate PR: pkukielka/sourcegraph-public-snapshot#1
Test plan
Setup:
pkukielka/add-external-auth-providers-support
branch.export CODE_SEARCH_DIR_OVERRIDE=/path/to/pkukielka/sourcegraph-public-snapshot
pkukielka/ext-auth-fix-search
branch) in Cody projec dircd jetbrains
./gradlew customRunIde -PforceCodeSearchBuild=true -PforceAgentBuild=true
Test
Option + S
to open sourcegraph searchOpen in Browser
"cody.override.serverEndpoint"
line and then switch to other account, e.g. on https://sg02.sourcegraphcloud.com/ (this one will authenticate using token), save settingsOpen in Browser
Reproducing failing fetches:
commandLine
line to:"commandLine": ["echo '{ \"headers\": { \"X-Forwarded-User\": \"SomeUser\" } }'"],
ide.browser.jcef.debug.port
9222
)Option + S
to open sourcegraph searchSourcegraph
link to inspect the failures in the devtools