Skip to content

Add opening on GitHub of live results variant analyses#1685

Merged
koesie10 merged 3 commits intomainfrom
koesie10/open-live-results-on-github
Oct 31, 2022
Merged

Add opening on GitHub of live results variant analyses#1685
koesie10 merged 3 commits intomainfrom
koesie10/open-live-results-on-github

Conversation

@koesie10
Copy link
Copy Markdown
Member

See the two separate commits: this adds the controllerRepo to the shared VariantAnalysis type and then uses it for constructing the actions workflow run URL.

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 adds the `controllerRepo` field to the `VariantAnalysis` shared
type. This is technically a breaking change since the old history won't
have this field and all calls on this will fail. However, the feature
is not available so this should be fine.
@koesie10 koesie10 requested review from a team as code owners October 31, 2022 13:22
@koesie10 koesie10 force-pushed the koesie10/open-live-results-on-github branch from bf4bc3c to 9b9ee44 Compare October 31, 2022 13:28
This implements the "Open on GitHub" context menu item for live results
variant analyses.
@koesie10 koesie10 force-pushed the koesie10/open-live-results-on-github branch from 9b9ee44 to 847cb13 Compare October 31, 2022 13:46
Copy link
Copy Markdown
Contributor

@charisk charisk left a comment

Choose a reason for hiding this comment

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

LGTM, just some minor comments.

@@ -1214,16 +1214,17 @@ export class QueryHistoryManager extends DisposableObject {
const { finalSingleItem, finalMultiSelect } = this.determineSelection(singleItem, multiSelect);

// Remote queries only
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.

Shall we remove this comment now?

it('should get the run url for remote query history items', () => {
const actionsWorkflowRunUrl = getActionsWorkflowRunUrl(remoteQueryHistoryItem);

expect(actionsWorkflowRunUrl).to.equal(`https://github.com/${remoteQueryHistoryItem.remoteQuery.controllerRepository.owner}/${remoteQueryHistoryItem.remoteQuery.controllerRepository.name}/actions/runs/${remoteQueryHistoryItem.remoteQuery.actionsWorkflowRunId}`);
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.

(optional) This is a very long line, it'd be nice to break things up a bit e.g. extract some of the item used in the URL into variables.

This removes a comment and makes the test lines shorter.
@koesie10 koesie10 merged commit 453cc77 into main Oct 31, 2022
@koesie10 koesie10 deleted the koesie10/open-live-results-on-github branch October 31, 2022 15:20
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