Skip to content

Split codeQL.copyVariantAnalysisRepoList into two commands#2646

Merged
robertbrignull merged 5 commits intomainfrom
robertbrignull/fix_alert_461
Jul 28, 2023
Merged

Split codeQL.copyVariantAnalysisRepoList into two commands#2646
robertbrignull merged 5 commits intomainfrom
robertbrignull/fix_alert_461

Conversation

@robertbrignull
Copy link
Copy Markdown
Contributor

Fixes https://github.com/github/vscode-codeql/security/code-scanning/461

codeQL.copyVariantAnalysisRepoList is used from two places, but they're both internal so it's trivial to split the command into two.

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.
  • [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

@robertbrignull robertbrignull requested review from a team as code owners July 27, 2023 11:23
Copy link
Copy Markdown
Contributor

@shati-patel shati-patel left a comment

Choose a reason for hiding this comment

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

Seems reasonable! Though I'm slightly confused that we now have a lot of command identifiers referencing the same "action". I think it makes sense, since some of them are just the user-facing ones, others are the internal commands? 🤔

e.g. We already have codeQLQueryHistory.copyRepoList, so do we need to define codeQL.copyVariantAnalysisRepoListQueryHistory separately?

@robertbrignull robertbrignull force-pushed the robertbrignull/fix_alert_461 branch from a0111ed to 59958a5 Compare July 28, 2023 09:09
@robertbrignull
Copy link
Copy Markdown
Contributor Author

robertbrignull commented Jul 28, 2023

@shati-patel, you raise a very good question. Looking into it more, I agree we don't need to have codeQLQueryHistory.copyRepoList immediately delegate to codeQL.copyVariantAnalysisRepoListQueryHistory. In fact I don't think we gain anything by having these extra commands and we can delete codeQL.copyVariantAnalysisRepoListQueryHistory and codeQL.copyVariantAnalysisRepoListView. In both cases we can call VariantAnalysisManager.copyRepoListToClipboard directly and there's no reason not to.

The only thing we then have to consider is if we're still logging enough telemetry, so I've added in some telemetry when copying repo lists in the view. This mirrors what we do for exporting results which is defined just next to it. And in the query history case we're already executing codeQLQueryHistory.copyRepoList so we'll have already logged telemetry about that.

Copy link
Copy Markdown
Contributor

@shati-patel shati-patel left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this in some more detail! Looks good to me locally too ✨

EDIT: the failing test looks legit though 😅

@robertbrignull
Copy link
Copy Markdown
Contributor Author

the failing test looks legit though 😅

Yep, I wasn't thinking properly when I wrote that. It should be mocking the right function now 😅

@robertbrignull robertbrignull merged commit bb9299e into main Jul 28, 2023
@robertbrignull robertbrignull deleted the robertbrignull/fix_alert_461 branch July 28, 2023 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants