Skip to content

[monitorlib/rid injection] Enforce and fix UAS ID at TestFlight level #1006

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

Merged
merged 2 commits into from
Apr 8, 2025

Conversation

the-glu
Copy link
Contributor

@the-glu the-glu commented Mar 26, 2025

This PR partially addresses #1003 by fixing the UAS ID Field duplication in the TestFlight class, meaning it will be fixed when injecting a flight and when getting data from an injected flight.

A small typo has been fixed in checks, and a side small fix of filter_invalid_telemetry passed to the parent class has been done.

@the-glu
Copy link
Contributor Author

the-glu commented Mar 26, 2025

Remark:

It will also mean mock_uss will clean those field

@mickmis mickmis changed the title [monitorlib/rid injection] Enfore and fix UAS ID at TestFlight level [monitorlib/rid injection] Enforce and fix UAS ID at TestFlight level Apr 1, 2025
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.

So if the TestFlight is invalid with this change the constructor will start raising ValueError. That sounds reasonable in isolation however is this caught and handled correctly in callers?
When injecting a flight or getting data from an injected flight that sounds reasonable since that would be a configuration error.
However within the mock USS, or potentially other callers, this may not be appropriate. E.g. in mock USS for such an issue I would expect the mock USS to catch the exception and return an HTTP bad param response.
Have you checked those things?

Other than that the change LGTM.

@the-glu
Copy link
Contributor Author

the-glu commented Apr 2, 2025

So if the TestFlight is invalid with this change the constructor will start raising ValueError. That sounds reasonable in isolation however is this caught and handled correctly in callers? When injecting a flight or getting data from an injected flight that sounds reasonable since that would be a configuration error. However within the mock USS, or potentially other callers, this may not be appropriate. E.g. in mock USS for such an issue I would expect the mock USS to catch the exception and return an HTTP bad param response. Have you checked those things?

Seems mock USS handle it already and return the response:

image

TestFlight doesn't seems to be used in others situation, so I do assume we're fine?

@the-glu the-glu requested a review from mickmis April 2, 2025 10:21
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.

Thanks for checking 👍

@mickmis mickmis merged commit 1406c48 into interuss:main Apr 8, 2025
21 checks passed
@mickmis mickmis deleted the partial_1003_fix branch April 8, 2025 08:49
github-actions bot added a commit that referenced this pull request Apr 8, 2025
github-actions bot added a commit that referenced this pull request Apr 9, 2025
github-actions bot added a commit that referenced this pull request Apr 9, 2025
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