Skip to content
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

Simplify jetbrains account management #6558

Merged
merged 2 commits into from
Jan 10, 2025

Conversation

pkukielka
Copy link
Contributor

@pkukielka pkukielka commented Jan 8, 2025

Fixes https://linear.app/sourcegraph/issue/CODY-4643/
Fixes https://linear.app/sourcegraph/issue/QA-188/jetbrains-adding-password-in-keychain-will-logout-user-when-same

Retargetting PR from old repo (with some further simplifications):
sourcegraph/jetbrains#2817

Changes

This PR removes custom account management code from JetBrains.

Now account changing is possible only through webview.
JetBrains receives notifications about endpoint change and caches it locally.
JetBrains is also still responsible for storing token tokens through secrets/* API.

But during JetBrains startup we don't specfify current endpoint or token anymore.
Instead we allow cody to manage them by itself. This makes agent fully responsible for auth management. The only exception are tests, where we starts agent with pre-configured endpoint/token.

Test plan

  1. Automatic tests should be passing
  2. Full QA is needed, this PR should not introduce any change in the behaviour

Changelog

@pkukielka pkukielka force-pushed the pkukielka/simplify-jetbrains-acc-management branch 2 times, most recently from 00cc95f to 5ee8c9f Compare January 8, 2025 16:13
if (ConfigUtil.isCodyEnabled() && !ConfigUtil.isIntegrationTestModeEnabled()) {
CodyAgentService.getInstance(project).startAgent(project)
EndOfTrialNotificationScheduler.createAndStart(project)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This we should not need anymore.

Copy link
Contributor

@abeatrix abeatrix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code change looks straight-foward to me.
Non-blocker question: I'm not familiar with the jb setup - do we have any e2e test that cover the authentication process that cover this new update?

@pkukielka
Copy link
Contributor Author

pkukielka commented Jan 10, 2025

The code change looks straight-foward to me. Non-blocker question: I'm not familiar with the jb setup - do we have any e2e test that cover the authentication process that cover this new update?

Good question. I added an agent test to prove it works:

https://github.com/sourcegraph/cody/pull/6558/files#diff-17e0fe25a58947b1b2d04dcf93cdc54ac0d6255e4c39d1c4948f2f0131903935

@pkukielka pkukielka force-pushed the pkukielka/simplify-jetbrains-acc-management branch from 5ee8c9f to cff7fb6 Compare January 10, 2025 11:29
@pkukielka pkukielka force-pushed the pkukielka/simplify-jetbrains-acc-management branch from cff7fb6 to fc591a2 Compare January 10, 2025 11:37
@pkukielka pkukielka merged commit d90c851 into main Jan 10, 2025
21 of 22 checks passed
@pkukielka pkukielka deleted the pkukielka/simplify-jetbrains-acc-management branch January 10, 2025 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants