Skip to content

Implement requestRepoResults message#1572

Merged
koesie10 merged 9 commits intomainfrom
koesie10/request-repo-results-message
Oct 11, 2022
Merged

Implement requestRepoResults message#1572
koesie10 merged 9 commits intomainfrom
koesie10/request-repo-results-message

Conversation

@koesie10
Copy link
Copy Markdown
Member

@koesie10 koesie10 commented Oct 7, 2022

This is a basic implementation of the requestRepoResults message. It currently does not work since results are not yet unzipped, but if the files are present, this should fully work. However, I've not been able to test this.

It's probably easiest to review this PR commit-by-commit.

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.

This will store all variant analysis that are run in the manager. Right
now, it only stores the variant analyses in memory. In the future, these
will be loaded from the query history and can be restored after a
restart.
In most cases, we will not have access to the full repo task object
since this needs to be retrieved from the API. Since we are only using
the full name from the repo task object, we can just use the full name
instead.
To create the interpreted and raw results from the SARIF/BQRS files, we
need some information from the repo task object. This will store the
repo task object to the filesystem as JSON so we can read them when
loading results.
This adds a new VSCode command which can be used to load results.
@koesie10 koesie10 added the secexp label Oct 7, 2022
Base automatically changed from koesie10/variant-analysis-results-manager to main October 7, 2022 12:08
@koesie10 koesie10 marked this pull request as ready for review October 7, 2022 12:30
@koesie10 koesie10 requested review from a team as code owners October 7, 2022 12:30
Copy link
Copy Markdown
Contributor

@robertbrignull robertbrignull left a comment

Choose a reason for hiding this comment

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

Code looks good to me.

public async loadResults(variantAnalysisId: number, repositoryFullName: string): Promise<void> {
const variantAnalysis = this.variantAnalyses.get(variantAnalysisId);
if (!variantAnalysis) {
throw new Error(`No variant analysis with id: ${variantAnalysisId}`);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Out of interest, how would this error surface to the user? Would it appear as a notification or be in a debug window or be completely hidden?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Since this is called by a command runner, this will show a warning notification and be logged (using the showAndLogWarningMessage function).

@koesie10 koesie10 merged commit c90eede into main Oct 11, 2022
@koesie10 koesie10 deleted the koesie10/request-repo-results-message branch October 11, 2022 14:48
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