search

svix / svix-webhooks

The enterprise-ready webhooks service 🦀
https://www.svix.com
MIT License
2.4k stars 166 forks source link

Libs(Go): template overrides for nullable non-required containers #1450

Closed svix-onelson closed 1 month ago

svix-onelson commented 1 month ago

In #1448 the EndpointIn model was hand-edited such that channels and filterTypes were nullable, and thus correctly omitted from request bodies sent to the server.

This series of commits attempts to remove the need to keep reapplying this patch with each spec bump by updating the codegen template used to produce the model code.

This change is comprised of 3 commits:

  1. the addition of a template override for model_simple (src) as-is, taken from openapi at the v5.2.0 tag (matching the version of the generator we're using currently).
  2. some modifications to said template, i.e. our patch.
  3. updated go sources from running the generator.

When reviewing the template, it'll therefore be useful to go commit by commit so you can see the diff applied against the upstream.

This change has widespread impact as many models are changing. I'm still trying to figure out how we can prove everything is kosher and not causing accidental breakage. Open to ideas 🤔

Existing tests are passing, and everything typechecks, so at the very least the codegen output is self-consistent.

svix-james commented 1 month ago

It'd be great to have some basic tests before merging this / confirmation that they're passing. They don't need to be wired into CI now, but just having some that can be run manually would be a good starting point.

svix-onelson commented 1 month ago

It'd be great to have some basic tests before merging this / confirmation that they're passing. They don't need to be wired into CI now, but just having some that can be run manually would be a good starting point.

I raised https://github.com/svix/svix-webhooks/pull/1452 which will run any Go tests for this in CI.

Will need to add more tests to cover the 27 model sources impacted, and like you suggested, they may need to be skipped in CI until we get the infra side of this figured out.

It'll take me a minute to get that going. In the meantime, #1452 should de-risk an accidental revert of the hand-edit for EndpointIn.

svix-james commented 1 month ago

So do you want this merged now, or hold off?

svix-onelson commented 1 month ago

So do you want this merged now, or hold off?

Hold off.