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

Check in updated swagger API contract for checks to influxdb #14284

Closed
ebb-tide opened this issue Jul 9, 2019 · 3 comments · Fixed by #14324
Closed

Check in updated swagger API contract for checks to influxdb #14284

ebb-tide opened this issue Jul 9, 2019 · 3 comments · Fixed by #14324
Assignees
Milestone

Comments

@ebb-tide
Copy link
Contributor

ebb-tide commented Jul 9, 2019

Swagger API contract needs to be in influxdb master in order to generate types for influxdb js client.

@ebb-tide ebb-tide changed the title Check in updated swagger API contract for alerts to influxdb Check in updated swagger API contract for checks to influxdb Jul 9, 2019
@ebb-tide ebb-tide added this to the catseye milestone Jul 9, 2019
@ebb-tide ebb-tide self-assigned this Jul 9, 2019
@chnn
Copy link
Contributor

chnn commented Jul 9, 2019

It seems like the alerts + notifications work will generate a steady stream of API changes, and it would be exhausting to have each change go through the whole cycle:

  1. Checkout client
  2. Regenerate swagger
  3. Open client PR and get it reviewed
  4. Merge client PR
  5. Publish new version
  6. Open PR in InfluxDB to integrate new client version + get it reviewed

This might be a good time to rethink this workflow. We talked at retro recently about moving the client back into the influxdb repo, since it's so painful / slow to keep it separate.

Also somewhat unrelated, but the types generated by openapi-generator are really poor—especially when it comes to OpenAPI enum types, of which there are many in the alerts + notifications API. Since we rely on types so much now for correctness and to catch breaking changes, I think it would be worthwhile to see if we can improve the type generation. I spiked on a OpenAPI spec to types generator a while back and it seemed pretty straightforward.

@ebb-tide @121watts Thoughts?

Connect #14285

@121watts
Copy link
Contributor

121watts commented Jul 9, 2019

@chnn I would love to move the client into influxdb. The problems it solved are outweighed by the costs of its maintenance. Are you suggesting rolling our own types generator based on the OpenAPI spec?

@chnn
Copy link
Contributor

chnn commented Jul 9, 2019

Are you suggesting rolling our own types generator based on the OpenAPI spec?

Yep. I think there's two pain points right now:

  1. We generate the client from the API spec in a separate repo, which complicates our workflow
  2. The API generation isn't very good, so we (shoddily) wrap the generated output with handwritten types and utilities

I think our own generator would solve (2) only, but it is by no means necessary for solving (1).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants