Make the monitoring command slightly simpler and handle being called on any variant analysis

[robertbrignull]

This PR aims to make the variant analysis monitoring command able to handle all states and also be slightly cleaner in its logic. I don't think any of the changes in this PR are critical, but I think they are all improvements.

The changes made are:

• Instead of firing _onVariantAnalysisChange from three different places under various conditions, fire it from one place and always fire it in every loop right after getting the updated variant analysis summary.
  • I'm not sure why we fired the event before the loop. This is before we fetched the new updated state so we're just emitting the old state that the monitor was passed when it started up. In the case of rehydrating query history this state could be very old. My only thought is that it's to ensure that the event always fires at least once. Ignoring a HTTP error I think this PR still does that so I think that's fine.
• Try to available download results regardless of if the variant analysis has a failure reason. I'm not sure if this matters as possibly we only set a failure reason for errors that happen before triggering the workflow run, but the idea is to always download available results regardless of other errors. If there are some results available then we should show them to the user.
• Reduces the VariantAnalysisMonitorResult to only the cases that we actually use, and make it fit better to the variant analysis summary status.
  • There were various cases in that type that we never used, so it seems sensible to remove them.
  • There were other cases that I didn't really understand. What does it mean for the monitor to "complete unsuccessfully"?
  • This type is only used for this one return value, and in the production code this return value is not used. It's only read from tests as a way of verifying the behaviour of the monitoring command.

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.

[koesie10]

LGTM, thanks for cleaning this up!

Since you mention the VariantAnalysisMonitorStatus is only used in tests, would it make sense to remove the return type completely and rely on the actual behaviour in the tests, rather than on the return type? It now seems like we're testing something that doesn't actually get used.

[robertbrignull]

Since you mention the VariantAnalysisMonitorStatus is only used in tests, would it make sense to remove the return type completely and rely on the actual behaviour in the tests, rather than on the return type? It now seems like we're testing something that doesn't actually get used.

I agree with you, but I didn't want to go too far with changing things in this PR. I'll open a tech-debt issue about it and we can discuss there.

[elenatanasoiu]

Is this different than if (failureReason) ?

[robertbrignull]

It is technically different because the empty string is falsy. Practically I'm not sure it'll make a difference.

If the failure message is set to the empty string that's not great, but I'd say it's a bug earlier in the system rather than here. So here I generally prefer to be explicit about checking for undefined rather than using the implicit conversion to a boolean.

[elenatanasoiu]

Had one question, but looks great!