fix(http/swagger): repair DBRPs type to match implementation #20705
Conversation
sranka
force-pushed
the
fix/swagger_dbrps_type
branch
from
61571b2 to
f1299a4
Compare
There was a problem hiding this comment.
We normally consider the API spec to be the source of truth, and fix the implementation to match when the two don't align. Agreed on the need to rename the key in the DBRPs schema, but we'll need to coordinate with the Cloud team to make the same change there.
| @@ -12036,12 +12036,10 @@ components: | |||
| - retention_policy | |||
| DBRPs: | |||
| properties: | |||
| notificationEndpoints: | |||
| content: | |||
There was a problem hiding this comment.
To match the convention of other schemas, I think this should be dbrps
There was a problem hiding this comment.
This is just a copy-paste error in the swagger file. The existing influx v1 auth command already uses the content property and it works fine in OSS and in the cloud, the client also uses https://github.com/influxdata/influxdb/blob/master/dbrp/http_server_dbrp.go#L128. IMHO it would be better to fix it to match the implementation state first and then possibly create a defect for a better name and links later as a second step if you want to. It does not create any regression and nobody has to look left and right to proceed, WDYT?
There was a problem hiding this comment.
Yeah, with fresh eyes it sounds like a good idea to fix for now and then improve later 👍
| type: array | ||
| items: | ||
| $ref: "#/components/schemas/DBRP" | ||
| links: | ||
| $ref: "#/components/schemas/Links" |
There was a problem hiding this comment.
This seems standard in many other objects returned by the API, I think we should instead try to fix the implementation to fill it in.
This PR repairs swagger DBRPs type to match actual implementation in https://github.com/influxdata/influxdb/blob/master/dbrp/http_server_dbrp.go#L128
notificationEndpointsproperty a was copy-paste mistake, it is calledcontentlinksproperty removed, it is not returned by the APIThis change is also required for InfluxDB client libraries that rely upon code generated from the swagger file.