-
Notifications
You must be signed in to change notification settings - Fork 19
Creation of initial decision records. #959
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
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
# API Versioning and Reporting | ||
|
||
## Summary | ||
|
||
API should provide a reason version and list/matrix of capabilities for a given instance of CDA | ||
|
||
## Opinions | ||
|
||
### Opinion 1 Calendar versioning | ||
|
||
@MikeNeilson | ||
|
||
Summary: Calendar versioning is easier to support and automate with this API | ||
|
||
As the one whose has been making the "official" releases Semantic Version versioning has basically been useless. | ||
We have been making so many feature additions that if I was doing it right we'd never have a minor version change between releases. | ||
It's also caused me to, I think, release too slowly. | ||
|
||
While we can automate SemVer it is an additional step. | ||
|
||
With Calendar Versioning automation tools can just pick the current date when appropriately triggered, | ||
perhaps by merged into a particular branch. | ||
|
||
|
||
### Opinion 2 Users | ||
|
||
Summary: It has been asked more than once that a version be provided | ||
|
||
Having a version allows client to better respond to what's available instead of failing in obtuse ways. | ||
|
||
|
||
## Decision Status | ||
|
||
accepted | ||
|
||
1. Provide endpoint to retrieve current API version. | ||
2. Likely include capability list or matrix. | ||
|
||
## References | ||
|
||
|
||
## Related decisions | ||
|
||
- data-versioning |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
# Data Types use Calendar Versioning | ||
|
||
## Summary | ||
|
||
Instead of versioning the entire API, though the API has a version, we version the data types. | ||
|
||
## Opinions | ||
|
||
### Opinion 1 | ||
|
||
Summary: Versioning the API itself, at the level of the path, will lead to too many paths to manage and be awkward for the clients | ||
|
||
@MikeNeilson | ||
|
||
By versioning the data, and using the Content-Type and Accept headers and the full features of MIME types we appropriately | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think REST principal accuracy is a good goal, but I think we should consider the following:
I would argue that accept header versioning provides the same amount of clutter both in implementation and downstream usage as path parameter versioning. However, it fails at the other three points. While I would not want to have a tool (Swagger webpage) dictate how we implement an API, I do think it is a non-trivial point when it is our only source of API documentation. Path versioning on the other hand, communicates very clearly to end users what different versions' behaviors will be, both in query parameter behaviors and in what formats are accepted/returned. Switching to calendar versioning on the accept header gives away the API clutter benefit as now downstream usages will have to manage many different versions beyond easy-to-grok integers, in which case, might as well put the version in the path or as a query parameter to at least provide easier documentation and browser support. I could see a slight mitigation for the explosion of versions to manage by looping it into the release cycle so they are at least versioned along with the REST API itself, rather than at PR creation date. At that point though, we're just adding more maintenance burden and over time with faster release cycles this becomes moot. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note: we are expanding the API documentation, Actually I need to move these to rst and that read-the-docs infrastructure. But I'm still not convinced path versioning is correct. I can see the point about not using dates since they will be somewhat all over the place. But while path version may be an industry standard... it seems a really sloppy industry standard. Over time, while our outputs have varied slightly, the inputs have only been expanded, not broken in a backwards incompatible way (at least intentionally) And the few places were they have drastically change, when ended up with a better name of the endpoint anyways, usually something more refined and specific that I have found some arguments and discussion in favor of versioning data, I will take the time to look them up. Another option, instead of |
||
separate the concern of "what data we are retrieving/storing" from the presentation of data. | ||
|
||
e.g /timeseries/Alder Springs.Temp-Air.Inst.15Minutes.0.GOES-raw?begin=PT0&end=PT-1D&units=C, granted with a reasonable | ||
exception to the units, defines *what* we want. | ||
|
||
The header, Accept, informs the API what format, or formats, we are willing to accept the data in. | ||
|
||
|
||
|
||
|
||
## Decision Status | ||
|
||
proposed | ||
|
||
Data, by content-types, are versioned. In the past there was some severe confusion on this part and it was treated as anything new was "version=2" in the content-type. To allow this design but reduce confusion going forward | ||
|
||
1. The initial version of a data set *SHALL* be the date it was finalized (e.g. PR about to be merged.) | ||
2. *IF* it is the first version of this data the plain content-type "e.g. application/json" will point to this data. | ||
3. *IF* is it not the version version of this data, it will be discussed and announced when it becomes the new default data. | ||
4. Downstream systems *SHOULD* use the specific version regardless of when implemented, and this behavior should be well documented. | ||
5. If a given data set includes definitions of its shape within the type there should be sufficient documentation for downstream | ||
developers to properly account for any changes over time. (See our TimeSeries type and discussions within #927). | ||
|
||
Version format is `YYYY-MM-DD` | ||
|
||
[comment:] <> (Status: request for comments | proposed | accepted | rejected | deprecated | superseded) | ||
|
||
## References | ||
|
||
I have several, I will dig them up likely next week | ||
|
||
## Notes | ||
|
||
The initial idea in CDA was that the first version of any data type was, we'll just stick with JSON for each of message, | ||
"application/json;version=1" with "application/json" being the alias to the latest format version. However, this was not | ||
correctly communicated and several brand new data transfer objects were created as ";version=2" under the impression that | ||
this was the version for the new system. Attempting to use a simple number of this has clearly caused confusion in general. | ||
|
||
We also failed to create the initial alias system which caused even more confusion when users attempted to test things | ||
directly in a browser instead of the provided swagger-ui. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
# Data should be readily searchable | ||
|
||
## Summary | ||
|
||
It's not just a good idea, it's technically the law. While CDA currently expose a fair amount of information to search | ||
it's never entirely clear. | ||
|
||
## Opinions | ||
|
||
### Opinion 1 | ||
|
||
Summary: Each data time should support it's own /catalog end point. | ||
|
||
@MikeNeilson | ||
|
||
The original CDA has the say, `/timeseries` end point provide a catalog if no data is set. I created a /catalog end point | ||
to attempt to consildate search query parameters. For TimeSeries and Locations this works reasonably well since there | ||
is parity between the concepts. | ||
|
||
However, if we tried to add ratings into the mix, the list of query parameters grows, and it would rather difficult to | ||
document which is for what or what changes for each. | ||
|
||
To make 'catalog' operations clear, we should create /catalog for each data type that provide for discoverability of that data. | ||
|
||
### Opinion 2 | ||
|
||
Summary: Each datatype under "catalog" should be a full path" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. trailing quote |
||
|
||
@MikeNeilson | ||
|
||
If it makes sense to group all catalogs under catalog, perhaps for grouping in the SWAGGER-UI, making each catalog it's own | ||
path under `/catalog` instead of the current path parameter is a better approach. | ||
|
||
We would maintain the grouping, but each catalog can have it's appropriate search criteria. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This does seem in opposition to the accept header reasoning. We already implement getAll for most (all?) data types. However, most of them return the full data object rather than a listing of refs/ids. The catalog endpoint is supposed to be the lightweight alternative to a getAll, which seems to be a change in the shape of the data. A There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair point, I hadn't thought of just using a different content-type of content-type parameter in that situation. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will say, while I brought up using the header, I still don't like that solution given my arguments above about discoverability and lack of clear Swagger UI documentation.... |
||
|
||
## Decision Status | ||
|
||
proposed | ||
|
||
[comment:] <> (Status: request for comments | proposed | accepted | rejected | deprecated | superseded) | ||
|
||
## References |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
# Title | ||
|
||
## Summary | ||
|
||
## Opinions | ||
|
||
### Opinion 1 | ||
|
||
Summary: | ||
|
||
Author | ||
|
||
descriptive text | ||
|
||
## Decision Status | ||
|
||
[comment:] <> (Status: request for comments | proposed | accepted | rejected | deprecated | superseded) | ||
|
||
## References |
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.
Definitely agree!