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

[openapi] move or add fields from injection.yaml to commons.yaml #25

Merged
merged 4 commits into from
Nov 6, 2024

Conversation

Shastick
Copy link
Contributor

@Shastick Shastick commented Nov 5, 2024

To prepare for some changes to the injection and observation API that is required for interuss/monitoring#754, this PR moves and adds some object definitions to commons.yaml, so they may be referenced from both injection.yaml and observation.yaml in subsequent PRs.

Definitions and behaviors should still be equivalent to before.

minimum: 0
exclusiveMinimum: false
type: number
UAType:
Copy link

Choose a reason for hiding this comment

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

In injection.yaml this is known as RIDAircraftType. Reason is that RIDAircraftType is taken from v19 and UAType is taken from v22a. The only practical difference seems that in v22a the value VTOL is not present anymore.
However now for the testing interfaces we want a single interface and not two different ones.

Here I'd be tempted to:

  • remove RIDAircraftType from injection.yaml
  • reference the common UAType here
  • add here the VTOL value with a note

That should result in a compatible API. WDYT? Maybe another subject for the weekly today.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interestingly, while RIDAircraftType is defined in injection.yaml, it is not used anywhere in the actual API definition (meaning we can change it at will without breaking anything)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed at the weekly meeting, will add the VTOL enum to UAType, and only keep UAType for the injection.

It will be the USS's job to map this to the v19 version of things if they are implementing that version of the standard.

rid/v1/commons.yaml Show resolved Hide resolved
rid/v1/commons.yaml Show resolved Hide resolved
@Shastick
Copy link
Contributor Author

Shastick commented Nov 5, 2024

@mickmis updated as discussed at the meeting (keep UAType, add VTOL to it, remove RIDAircraftType)

Copy link

@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.

I realized that there is one more difference for some fields between their v19 and v22a versions: their default values. Given that it is a (subtle) behavior change of an interface I think it's important to isolate that in a separate PR/commit with a version increment.

Example in this PR:

  • old HorizontalAccuracy: v19, new: v22a
  • old VerticalAccuracy: v19, new: v22a
  • old and new SpeedAccuracy: v19

So I propose the following course of action:

  • Update this PR that moves/add schemas so that there is no behavior change at all (i.e. keep the v19 versions, including RIDAircraftType as is)
  • Add a new PR migrating schemas to v22a (including RIDAircraftType -> UAType with the VTOL exception), there do a version increment

… the following commits that update the version)
Copy link

@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.

LGTM thanks

@mickmis mickmis changed the title [openapi] move or add fields to commons.yaml for upcomming change [openapi] move or add fields from injection.yaml to commons.yaml Nov 6, 2024
@mickmis mickmis merged commit bc5fa08 into interuss:main Nov 6, 2024
@mickmis mickmis deleted the common-refactor branch November 6, 2024 14:46
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