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

Overridable OpenAPI Spec #140

Merged
merged 22 commits into from
Jan 17, 2022
Merged

Conversation

ashwinath
Copy link
Contributor

@ashwinath ashwinath commented Dec 2, 2021

This PR allows overriding of OpenAPI spec in the turing charts so that we can change the experiment type enum without maintaining a separate set of openapi specificiations in our private repository. The other motivation is that we are now able to override the servers[].url to allow custom domains other than http://localhost:8080/v1.

However, in this PR, we have generalised it so that we can override any portion of the OpenAPI specification as long as it follows the OAS3 specification.

There is no longer a need to copy over the raw OpenAPI spec files (handwritten) and both the validation and the OpenAPI UI will use the same bundled OpenAPI spec file.

In the Github Actions, we now compile the OpenAPI into one bundle file and put it into the Turing API image instead of building it during the uber Turing API image step as it is required for the E2E tests.

@ashwinath ashwinath changed the title Overridable openapi WIP: Overridable OpenAPI Spec Dec 2, 2021
@ashwinath ashwinath force-pushed the overridable-openapi branch 3 times, most recently from 8b5462b to d27254c Compare December 2, 2021 06:54
@ashwinath ashwinath changed the title WIP: Overridable OpenAPI Spec Overridable OpenAPI Spec Dec 2, 2021
@ashwinath ashwinath requested a review from a team December 2, 2021 07:24
Copy link
Contributor

@romanwozniak romanwozniak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still a bit unclear with this PR as it mixes changes to OAS3 spec files AND SwaggerUI serving configuration. Can we decouple these two steps? I.e, can we do it in this way:

  1. When we build a turing-api image, can we also merge files from turing/api/api into a single YAML spec file and include it with the turing-api image (but we don't need Swagger UI at this point)
  2. When we build a turing image, we can generate swagger-ui-dist directory and point it to bundled spec file (from step 1)
  3. When turing-api server starts, it can merge existing openapi.bundle.yaml in-place with provided patch data

Dockerfile Show resolved Hide resolved
api/config-dev.yaml Show resolved Hide resolved
api/turing/config/config.go Outdated Show resolved Hide resolved
api/turing/config/config.go Outdated Show resolved Hide resolved
api/turing/config/config.go Outdated Show resolved Hide resolved
api/turing/config/config.go Show resolved Hide resolved
api/turing/config/testdata/config-2.yaml Show resolved Hide resolved
infra/charts/turing/templates/_helpers.tpl Outdated Show resolved Hide resolved
infra/e2e/turing.values.yaml Outdated Show resolved Hide resolved
@ashwinath
Copy link
Contributor Author

I'm still a bit unclear with this PR as it mixes changes to OAS3 spec files AND SwaggerUI serving configuration. Can we decouple these two steps? I.e, can we do it in this way:

1. When we build a `turing-api` image, can we also merge files from `turing/api/api` into a single YAML spec file and include it with the `turing-api` image (but we don't need Swagger UI at this point)

2. When we build a `turing` image, we can generate swagger-ui-dist directory and point it to bundled spec file (from step 1)

3. When turing-api server starts, it can merge existing `openapi.bundle.yaml` in-place with provided patch data

We can but I'm not sure how this solution would help. To me I only see the effects of having a smaller image during the Github Actions but I'm not sure if we gain anything significant. In terms of organisation wise I'm not sure if this does anything better also.

It also adds a lot of noise to this PR.

api/Dockerfile Outdated Show resolved Hide resolved
api/Dockerfile Outdated Show resolved Hide resolved
api/turing/config/config.go Outdated Show resolved Hide resolved
api/turing/config/example.yaml Outdated Show resolved Hide resolved
api/turing/server/api.go Outdated Show resolved Hide resolved
api/turing/server/application.go Outdated Show resolved Hide resolved
@ashwinath
Copy link
Contributor Author

@romanwozniak I have reworked the configuration into a much simpler way. The following changes have been made:

  1. openapi.bundle.yaml is commited we guarantee that the developer will have the bundle files. This can be regnerated by just running make in /api/api folder.
  2. You can provide an override yaml file. The reason why I chose file over configuration was because there is a bug/harmful feature introduced that lower cased all the keysUnmarshalKey normalizes all map keys to lower case spf13/viper#373), I could not use the fix because i needed to change to spf13/viper instead of ory/viper and i need at least go 1.16 to make it work
  3. Configuration is much easier now, it only introduces an additional config that takes in the override file.
  4. We serve the new merged in memory YAML, merged or not, and serve it under /static/openapi.bundle.yaml (configurable).

@romanwozniak
Copy link
Contributor

2. I could not use the fix because i needed to change to spf13/viper instead of ory/viper and i need at least go 1.16 to make it work

Now I wonder why do we use ory/viper in the first place (that's a rhetorical question)

scripts/swagger-ui-generator/swagger-ui-generator.sh Outdated Show resolved Hide resolved
api/turing/server/static.go Outdated Show resolved Hide resolved
api/turing/server/application.go Outdated Show resolved Hide resolved
api/turing/config/example.yaml Outdated Show resolved Hide resolved
api/turing/config/example.yaml Outdated Show resolved Hide resolved
api/turing/config/config.go Outdated Show resolved Hide resolved
@romanwozniak
Copy link
Contributor

Left some styling suggestions, but conceptually it lgtm

@ashwinath ashwinath merged commit 1de5d83 into caraml-dev:main Jan 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants