-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Ensure a request for telemetry happens in Condition Sets #7592
Ensure a request for telemetry happens in Condition Sets #7592
Conversation
…status-for-operatormission-status-on-nav
The PR in action: navigation.movNote we don't have a great way of writing e2e tests for this right now as our YAMCS setup doesn't have:
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7592 +/- ##
==========================================
+ Coverage 56.13% 56.15% +0.02%
==========================================
Files 672 672
Lines 27116 27122 +6
Branches 2635 2635
==========================================
+ Hits 15222 15231 +9
+ Misses 11567 11564 -3
Partials 327 327
... and 7 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a question
const latestTimestamp = getLatestTimestamp( | ||
{}, | ||
{}, | ||
this.timeSystems, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this function call looks insane lol
this.subscriptions[id] = this.openmct.telemetry.subscribe( | ||
|
||
// get latest telemetry value (in case subscription is cached and no new data is coming in) | ||
this.requestLatestValue(endpoint); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that'll fix it i reckon. nice
…status-for-operatormission-status-on-nav
…status-for-operatormission-status-on-nav
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Will file a followup to refactor Condition Sets to use TelemetryCollections which will remove a lot of boilerplate here.
Edit: Filed #7620
…status-for-operatormission-status-on-nav
Wonderful! This was my thought too, but seemed ambitious for a bugfix. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Closes #7484
Describe your changes:
When subscribing to telemetry in a Condition Set, we need to ensure we request telemetry in addition to a simple subscribe. This is because on telemetry like operator status, the Telemetry API may already have a subscription (due to the poll widget), and thus not create a new subscription, and thus not immediately fire a callback.
All Submissions:
Author Checklist
type:
label? Note: this is not necessarily the same as the original issue.Reviewer Checklist