Skip to content
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

[uss_qualifier] monitorlib: ISAChange exposes subscribers as returned by DSS #838

Merged

Conversation

Shastick
Copy link
Contributor

@Shastick Shastick commented Nov 3, 2024

This allows users of the DSSWrapper to easily check which subscribers were mentioned in a response to create, mutate or delete an ISA on the DSS.

Up to now this could be reconstructed by looking at the notifications that were effectively sent by the wrapper, but this was needlessly cumbersome and breaks down when the qualifier ignores notifications that should be sent to itself.

(This change was required by #839)

@Shastick Shastick force-pushed the rid-isa-change-expose-subscribers branch 2 times, most recently from b9c3b9e to 8e46667 Compare November 3, 2024 20:47
@Shastick Shastick marked this pull request as ready for review November 4, 2024 09:20
Copy link
Contributor

@mickmis mickmis left a comment

Choose a reason for hiding this comment

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

PR title: this looks more like a monitorlib change.

@@ -450,6 +449,9 @@ class ISAChange(ImplicitDict):
notifications: Dict[str, ISAChangeNotification]
"""Mapping from USS base URL to change notification query"""

subscribers: List[SubscriberToNotify]
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks this always contains dss_query.subscribers: rather than a new field why not adding a property returning that?

Copy link
Contributor Author

@Shastick Shastick Nov 5, 2024

Choose a reason for hiding this comment

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

Indeed, that is the straightforward solution

@@ -4,6 +4,7 @@
import arrow
import s2sphere
from uas_standards.astm.f3411 import v19, v22a
from zope.event import subscribers
Copy link
Contributor

Choose a reason for hiding this comment

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

That looks fishy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Auto-complete artifact, fishy indeed

@Shastick Shastick changed the title [uss_qualifier] rid: ISAChange exposes subscribers as returned by DSS [uss_qualifier] monitorlib: ISAChange exposes subscribers as returned by DSS Nov 5, 2024
@Shastick Shastick force-pushed the rid-isa-change-expose-subscribers branch from 8e46667 to d0813ae Compare November 5, 2024 10:56
@Shastick
Copy link
Contributor Author

Shastick commented Nov 5, 2024

Updated as suggested.

@mickmis mickmis merged commit 4f6ff0f into interuss:main Nov 5, 2024
20 checks passed
@mickmis mickmis deleted the rid-isa-change-expose-subscribers branch November 5, 2024 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants