-
Notifications
You must be signed in to change notification settings - Fork 87
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
[rid] Add v2 endpoints #784
[rid] Add v2 endpoints #784
Conversation
Also, improve missing scopes error message, fix RID v2 scope string, improve Volume4D parsing error messages
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.
Thank you for this update. Looks good to me. Few minor comments below while going through the review.
|
||
|
||
def test_ensure_clean_workspace_v2(ids, session_ridv2): | ||
print('In test_ensure_clean_workspace, scope = {}'.format(SCOPE_SP)) |
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.
Should be test_ensure_clean_workspace_v2
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.
It is actually the only print in all tests. You may want to remove it.
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.
Done
if err != nil { | ||
return nil, stacktrace.Propagate(err, "Error parsing upper altitude of Volume3D") | ||
} | ||
|
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.
Perhaps we would like to check that Exactly one outline must be specified.
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.
Done
return &ridpb.SubscriberToNotify{ | ||
Url: s.URL, | ||
Subscriptions: []*ridpb.SubscriptionState{ | ||
{ |
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.
Is it correct to assume that there is always only one subscription ?
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.
Added #789
return nil, stacktrace.NewErrorWithCode(dsserr.BadRequest, "Invalid ID format") | ||
} | ||
|
||
if !s.EnableHTTP { |
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.
I feel like this flag may benefit to be renamed since it is only used to disable URLs validation. I would propose to rename it as AllowHTTP or AllowHTTPBaseUrls to prevent confusion. Indeed, I got mislead thinking that this was linked to the listening protocol of the DSS server.
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.
The command option description is actually not accurate.
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.
Added #788
) | ||
|
||
if et := req.GetEarliestTime(); et != nil { | ||
ts := et.AsTime() |
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.
Would it makes sense to call first CheckValid
?
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.
Sure -- adjusted, and adjusted in v1 as well
} | ||
|
||
if lt := req.GetLatestTime(); lt != nil { | ||
ts := lt.AsTime() |
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.
Similar: Would it makes sense to call first CheckValid?
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.
Sure -- adjusted, and adjusted in v1 as well
This PR adds support for RID v2 (ASTM F3411 second version) endpoints, as well as prober tests to validate those endpoints.
Due to a limitation in the standards, v1 and v2 endpoints are only almost interoperable and not actually interoperable. The ideal solution to this problem would have been for the ASTM standards to specify protocols along with the URLs (e.g., "my v1 /flights endpoint is https://example.com/rid/v1/uss/flights", "my v2 base URL is https://example.com/rid/v2"), and this convention will hopefully be adopted in future versions of the standard. In the mean time, see the documentation in interfaces/rid/README.md for the approach taken in this DSS implementation.