Creation of initial decision records. #959
Conversation
MikeNeilson
requested review from
jbkolze,
rma-psmorris,
DanielTOsborne and
krowvin
|
I've tagged some specific people for initial review, please feel free to bring in anyone else that may have an opinion. This is a discussion of both the decision record format as well as the actual decisions being recorded. More will follow, I'm starting small...ish. |
|
@jvanaalsburg considering you started with with cwms-python, probably good to get your feedback about how I'm starting these as well. |
MikeNeilson
added
the
approved-W192HQ23F0232-task4
|
|
||
| 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. |
|
|
||
| @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.
I think REST principal accuracy is a good goal, but I think we should consider the following:
- API clutter
- industry standards
- browser support
- clear documentation
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.
The OpenAPI does not provide a meaningful way to distinguish between different versions on the version marker itself and the Swagger webpage does not make it immediately obvious that there are multplie versions of an endpoint.
Adding/removing/editing behavior of query parameters require extra documentation on the parameter (e.g. "This parameter is not supported by application/json;version=1, application/json;version=2 but is supported by application/json;version=2025-05-01. Now make sure you check what the default is to make sure the parameter is supported by your version."). Why do that to ourselves and users?
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.
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 /cwms-data/v1/locations -> /cwms-data/v2/locations (just for a concrete example).
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 ;version= could just be more expanded data types like application/vnd.json-ts-yada yada (I can't remember off the top of my end the right vnd syntax but I think that gets the idea across)
|
|
||
| ### Opinion 2 | ||
|
|
||
| Summary: Each datatype under "catalog" should be a full path" |
| 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.
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 application/json;catalog=true or something similar.
There was a problem hiding this comment.
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.
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....
MikeNeilson
removed
the
approved-W192HQ23F0232-task4
In reference to #942, this is the initial creation of decision records for CDA.
In the current state, as little feedback has been provided on #942 at this time, this is to get feedback on the format of the decision records themselves, but anyone looking at this PR should comment, maybe open PR to this PR to add opinions.
I do not intent to merge these until an actual decision is reached with appropriate feedback; however, barring feedback and discussion such decisions will be made.