-
Notifications
You must be signed in to change notification settings - Fork 8
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
Mdb overrides subscription #333
Conversation
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.
Hey! Nice! I've got a couple small questions/suggestions.
…o mdb-overrides-subscription
Testing Instructions:
|
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.
Nice! I have one more question that I think may be an existing issue in other subscription providers as well (definitely main realtime provider).
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 great! Just needs some tests. The same tests can verify both nasa/openmct#6735 and this.
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.
Have done some testing in our test environment. The changes work for catching limit changes immediately as they happen via subscription, but they do not resolve mdb overrides on load.
The limit-provider needs to also make a call to https://github.com/akhenry/openmct-yamcs/pull/333/files#diff-4a950de5fe10da4e7dff6cde433a8e4e90c4a1b606c6bb3919b79661c3302997L101 to resolve any MDB overrides on plot load.
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!
@@ -100,12 +101,13 @@ test.describe("Quickstart network requests @yamcs", () => { | |||
// wait for debounced requests in YAMCS Latest Telemetry Provider to finish | |||
await new Promise(resolve => setTimeout(resolve, 500)); | |||
filteredRequests = filterNonFetchRequests(networkRequests); | |||
|
|||
console.log(filteredRequests); |
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.
if you can remove this, we should. Not a big deal to print diagnostic to screen in tests
Verified (Overlay Plots) - Testathon - 7/20/23 Not sure if it's working for gauges or if that was a requirement. Not seeing anything there. |
Closes VIPEROMCT-295
Describe your changes:
Adds the subscribeToLimits API to the realtime provider
This subscribes to the type
mdb-changes
and returns any limits that have changed.All Submissions:
Author Checklist
Reviewer Checklist