Don't add database source archive folders by default #3047
Conversation
| makeSelected, | ||
| false, | ||
| ); | ||
| if (!addedDatabase) { |
There was a problem hiding this comment.
I assume we no longer need to treat the model editor as a special case, so I've removed the argument here.
There was a problem hiding this comment.
I'm not sure about this change: if you are in the model editor and have the setting enabled, we would now suddenly reload when you try to model a dependency. I think this may be unexpected because the user might not know that the database gets added as a normal database.
There was a problem hiding this comment.
You're right that this might be unexpected. Since it's less explicit that you're adding a database in this case, I'll keep the hard-coded "don't add source archive" setting! 👍🏽
extensions/ql-vscode/test/vscode-tests/minimal-workspace/local-queries/local-databases.test.ts
Outdated
Show resolved
Hide resolved
extensions/ql-vscode/src/config.ts
Outdated
| @@ -649,6 +649,15 @@ export function allowHttp(): boolean { | |||
| return ALLOW_HTTP_SETTING.getValue<boolean>() || false; | |||
| } | |||
|
|
|||
| const ADD_DATABASE_SOURCE_TO_WORKSPACE_SETTING = new Setting( | |||
| "addDatabaseSourceToWorkspace", | |||
| DATABASE_DOWNLOAD_SETTING, | |||
There was a problem hiding this comment.
Is this correct to be under databaseDownload? This also applies to database manually added by adding a folder or adding an archive, so it does apply to non-downloaded databases as well.
There was a problem hiding this comment.
Good point! I can rename the overall setting to something like "Adding databases" (instead of "Downloading databases")
| makeSelected, | ||
| false, | ||
| ); | ||
| if (!addedDatabase) { |
There was a problem hiding this comment.
I'm not sure about this change: if you are in the model editor and have the setting enabled, we would now suddenly reload when you try to model a dependency. I think this may be unexpected because the user might not know that the database gets added as a normal database.
|
(Converting back to draft while we wait for docs to be published and for merging to be unlocked) |
This reverts commit 56025d1.
I updated the user-facing name for this setting, but forgot about the internal variable
shati-patel
force-pushed
the
shati-patel/db-src-archive
branch
from
2294d8c to
5b1f528
Compare
shati-patel
force-pushed
the
shati-patel/db-src-archive
branch
from
5b1f528 to
c111a74
Compare
|
Docs are in place, and I've had confirmation that this is good to merge! (Apologies for all the commit spam, I had a few conflicts to resolve...) |
| "codeQL.databaseDownload.allowHttp": { | ||
| "type": "boolean", | ||
| "markdownDeprecationMessage": "**Deprecated**: Please use `#codeQL.addingDatabases.allowHttp#` instead.", | ||
| "deprecationMessage": "Deprecated: Please use codeQL.addingDatabases.allowHttp instead." | ||
| }, |
There was a problem hiding this comment.
As mentioned offline, users who still use this old setting name will get a deprecation message:
There was a problem hiding this comment.
For the record: we will no longer support the old setting name. This isn't the "formal" way of deprecating a setting, but this is a sufficiently niche/insecure setting that it shouldn't have much user impact.
In the case where a user did have this setting enabled: If they try to download a DB from an http location, they'll get the error message "Must use https for downloading a database.", which should point them to check their settings.
There was a problem hiding this comment.
Discussed offline as well, but this should be fine. As said it's not the full proper way of renaming a setting, and we could aim to support both for a transition period, but we expect this setting to be almost (if not completely) unused. And as Shati points out, if someone is using this the error should point them towards checking their settings and seeing the deprecation notice. This along with a changelog entry should mean that anyone who is using it sees it and updates their settings.
There was a problem hiding this comment.
LGTM
Thanks @shati-patel for adding the changelog / deprecation notice.
No longer add the database source folder to the workspace by default (since this caused bugs in single-folder workspaces). If users want to keep the old behaviour of auto-adding DB source folders, they can set the
addDatabaseSourceToWorkspacesetting totrue.See internal linked issue for more details.
Checklist
ready-for-doc-reviewlabel there.