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.

Sign up for GitHub

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

Jump to bottom

Add support for (de-)registering databases with the query server #681

Merged
merged 4 commits into from
Dec 2, 2020
Merged

Add support for (de-)registering databases with the query server #681

merged 4 commits into from
Dec 2, 2020

Conversation

aeisenberg
Copy link
Contributor

@aeisenberg aeisenberg commented Nov 18, 2020 •
edited
Loading

Sends commands to the query server to add and remove databases from the allow list. This feature will enable proper unlocking of databases when they are removed from vscode.

Fixes #572
Fixes #333
Fixes #249

Checklist

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • Issues have been created for any UI or other user-facing changes made by this pull request.
  • @github/docs-content-dsp has been cc'd in all issues for UI or other user-facing changes made by this pull request.

@aeisenberg aeisenberg marked this pull request as draft November 18, 2020 16:31
@aeisenberg
Copy link
Contributor Author

TODO: Ensure there is a version check before attempting to add or remove from the allow list. Older versions of the cli do not support this feature.

@aeisenberg aeisenberg closed this Nov 24, 2020
@aeisenberg aeisenberg deleted the aeisenberg/unlocking-db branch November 24, 2020 22:30
@aeisenberg
Copy link
Contributor Author

This was closed by mistake.

@aeisenberg aeisenberg restored the aeisenberg/unlocking-db branch November 25, 2020 23:36
@aeisenberg
Copy link
Contributor Author

Reopening.

@aeisenberg aeisenberg reopened this Nov 25, 2020
@aeisenberg aeisenberg mentioned this pull request Nov 27, 2020
Closed
@aeisenberg aeisenberg force-pushed the aeisenberg/unlocking-db branch 2 times, most recently from dab141c to 7e01187 Compare November 27, 2020 23:23
@aeisenberg aeisenberg changed the title Add support for the db allow list Add support for (de-)registering databases with the query server Nov 30, 2020
@aeisenberg aeisenberg marked this pull request as ready for review December 1, 2020 21:18
@aeisenberg
Copy link
Contributor Author

aeisenberg commented Dec 1, 2020 •
edited
Loading

Ping @github/docs-content-dsp!

With this change (and the corresponding change in the CLI) we ensure that databases removed from the workspace are unlocked and can be queried without restarting vscode. This is more of a bug fix than a new feature, so not sure if we need a new entry in the docs or if a mention in the changelog is sufficient.

Oh, and requires cli version 2.4.1 or later (so, not yet released).

@hubot
Copy link

hubot commented Dec 1, 2020 •
edited by shati-patel
Loading

:octocat:📚 Thanks for the docs ping! 🛎️ This was added to our docs first-responder project board. A team member will be along shortly to review this for docs impact, but you can also open a docs issue to request docs updates.

No docs impact!

@shati-patel
Copy link
Contributor

Ping @github/docs-content-dsp!

With this change (and the corresponding change in the CLI) we ensure that databases removed from the workspace are unlocked and can be queried without restarting vscode. This is more of a bug fix than a new feature, so not sure if we need a new entry in the docs or if a mention in the changelog is sufficient.

Oh, and requires cli version 2.4.1 or later (so, not yet released).

Thanks for the ping! You're right it's more of a bug fix, so a changelog entry should be sufficient 😊

@aeisenberg
Add version check for db registration
3144915 
Database registration is available in versions >= 2.4.1
@aeisenberg aeisenberg requested review from adityasharad, max-schaefer and alexet and removed request for max-schaefer December 1, 2020 22:42
adityasharad
adityasharad reviewed Dec 1, 2020
View reviewed changes
Copy link
Contributor

@adityasharad adityasharad left a comment

Choose a reason for hiding this comment

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

Generally looks good!

extensions/ql-vscode/CHANGELOG.md Outdated Show resolved Hide resolved
extensions/ql-vscode/src/databases-ui.ts Outdated Show resolved Hide resolved
extensions/ql-vscode/src/pure/messages.ts Outdated Show resolved Hide resolved
extensions/ql-vscode/src/queryserver-client.ts Outdated Show resolved Hide resolved
@aeisenberg
Pluralize registration message names
f9e2b08 
And do a version check before adding `--require-db-registration` flag.
adityasharad
adityasharad approved these changes Dec 2, 2020
View reviewed changes
Copy link
Contributor

@adityasharad adityasharad left a comment

Choose a reason for hiding this comment

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

Looks good!

@aeisenberg aeisenberg merged commit b6c7837 into github:main Dec 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adityasharad adityasharad adityasharad approved these changes

@alexet alexet Awaiting requested review from alexet

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@aeisenberg @hubot @shati-patel @adityasharad