Skip to content

Conversation

kmruiz
Copy link
Contributor

@kmruiz kmruiz commented Aug 8, 2023

Description

Try to move keytar loading into loadSavedConnections so it's only loaded once per migration.

Checklist

Motivation and Context

  • Bugfix
  • New feature
  • Dependency update
  • Misc

Open Questions

Dependents

Types of changes

  • Backport Needed
  • Patch (non-breaking change which fixes an issue)
  • Minor (non-breaking change which adds functionality)
  • Major (fix or feature that would cause existing functionality to change)

@kmruiz kmruiz requested a review from alenakhineika August 8, 2023 14:03
@kmruiz
Copy link
Contributor Author

kmruiz commented Aug 8, 2023

@alenakhineika that was the tests that were failing also on my local, they don't seem to be flaky, but some change of behaviour when we moved keytar to loadSavedConnections:

https://github.com/mongodb-js/vscode/actions/runs/5798050950/job/15714958344?pr=574#step:9:1290

Any clue? I personally won't put much more effort on this if the fix of the tests are too complex, as this is going to be run theoretically once during the migration and later we can just remove these 3 lines of code.

I won't be too concerned about the performance impact of this, as people don't have thousands of connections to migrate, probably just less than 5/10 and the storages are going to be far slower than failing loading the module.

@kmruiz kmruiz self-assigned this Aug 8, 2023
@alenakhineika
Copy link
Contributor

We do want to ensure that we require keytar once per extension activation to provide an accurate telemetry for VSCode team. The CI failure that you experienced is not related to the code moving you did. This started to happen because of a new VSCode release between your PRs, that introduced DNS result order changes. I have merged a fix for this issue, so rebase from main to unblock your PR.

@alenakhineika alenakhineika changed the title chore: move keytar to loadSavedConnections chore: move keytar to loadSavedConnections VSCODE-450 Aug 10, 2023
@kmruiz kmruiz merged commit 093d341 into main Aug 14, 2023
@kmruiz kmruiz deleted the chore/optimize-keytar-loading branch August 14, 2023 09:43
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.

4 participants