Push decision to use credentials or not down to where the creds are used#3440
Merged
robertbrignull merged 2 commits intomainfrom Mar 6, 2024
Merged
Push decision to use credentials or not down to where the creds are used#3440robertbrignull merged 2 commits intomainfrom
robertbrignull merged 2 commits intomainfrom
Conversation
koesie10
approved these changes
Mar 6, 2024
Member
koesie10
left a comment
There was a problem hiding this comment.
LGTM. I agree that we shouldn't be using the credentials in the model editor in non-canary mode, especially since this would only result in a credentials prompt when using flow generation.
Contributor
Author
|
I realised that this is a user-visible change so it should probably have a changelog entry. I hope that's ok. If you disagree we can editor or remove it again. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This fixes a potential bug I spotted where we might unexpectedly prompt a user for credentials to download a CodeQL database.
The code for downloading databases from GitHub is in
promptImportGithubDatabaseanddownloadGitHubDatabaseand both of those took an optionalCredentialsparameter. It is then up to the caller to decide whether to use credentials or not. In this PR we push the decision further down so we only decide indownloadGitHubDatabaseand therefore use the same logic for all cases.The current logic around using credentials is:
handleChooseDatabaseGithubinlocal-databases-ui.ts(called when manually importing a database by command palette or UI), we were using credentials only when the canary flag was set.createSkeletonQueryinlocal-queries.ts, we were using credentials only when the canary flag was set.promptImportDatabaseinmodel-editor-view.ts(called when modeling a dependency or generating modeled methods), we were always using credentials.This seems like a bug to me as the rest of the model editor doesn't require canary mode, and in all other instances of downloading a database it doesn't prompt for credentials when not in canary mode.
Checklist
ready-for-doc-reviewlabel there.