Skip to content

Bump openapi-generator #251

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

Merged
merged 2 commits into from
Mar 23, 2023
Merged

Bump openapi-generator #251

merged 2 commits into from
Mar 23, 2023

Conversation

dd-ashishaev
Copy link
Contributor

@dd-ashishaev dd-ashishaev commented Mar 22, 2023

What does this PR do?

  • Bump openapi-generator to the latest release

Description of the Change

We generate a Java client for one of our services. Unfortunately, the version that is used currently has a bug in the generator, and there's no good workaround. The fix is in this PR: OpenAPITools/openapi-generator#12131.

Alternate Designs

  • We can also bump the image locally, fork the generator and keep it version controlled in our repository. However, this means we have to update these templates manually later.

Possible Drawbacks

  • If other users of apigentools don't pin their version of apigentools, their templates will be updated.
  • In our service, we'll likely have to adjust our configs to accomodate changes in the new openapi-generator releases.
  • On the other hand, I think we should still update openapi-generator as its maintainers update templates and may add improvements.

Verification Process

  • Built the image locally: docker build . --tag apigentools:local
  • Configured our code generation script to use the local image.
  • Generated the Java client.
  • Ran a code snippet that tests our specific bug

Additional Notes

It would be practical to have a way to version templates independently from apigentools, and preferably per-language. Sometime a bug, like this one, requires an upgrade for one language, but other languages are operational.

Release Notes

Review checklist (to be filled by reviewers)

  • Feature or bug fix MUST have appropriate tests (unit, integration, etc...)
  • PR title must be written as a CHANGELOG entry (see why)
  • Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
  • PR must have one changelog/ label attached. If applicable it should have the backward-incompatible label attached.
  • PR should not have do-not-merge/ label attached.
  • If Applicable, issue must have kind/ and severity/ labels attached at least.

@dd-ashishaev dd-ashishaev marked this pull request as ready for review March 22, 2023 16:44
@dd-ashishaev dd-ashishaev requested a review from a team as a code owner March 22, 2023 16:44
@therve
Copy link
Contributor

therve commented Mar 23, 2023

For the record generally they way to update one template is to use a downstream patch: https://apigentools.readthedocs.io/en/latest/workflow/#add-template-patches

@dd-ashishaev
Copy link
Contributor Author

Thanks for your response! Template patches are great! On the other hand, I'm not sure they are a replacement for updating the templates in general. We could indeed copy the code from the linked PR and check it in our repository as a patch. However, this will become complicated if we need more changes, or need to understand where a generated line comes from. The patch will also separate the change from its context (state of the repo, PR/issue discussion).

In addition to that, not all bugfixes can be applied as a template patch/override, for example, if the changes are outside the templates in the code generation logic. You can fork the template in this case, but the maintenance burden of this is likely huge.

I'm OK with patching for now, but not sure this is the best way in general. What are your thoughts on bumping the version? I'm seeing that it hasn't been updated in about a year...

@therve
Copy link
Contributor

therve commented Mar 23, 2023

Oh no I was just suggesting that in the future. No issue from me updating the image.

@therve therve merged commit 878c4f2 into DataDog:master Mar 23, 2023
@dd-ashishaev
Copy link
Contributor Author

Ah! Thank you! Perfect 🙇 I think I misunderstood your comment, sorry!

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.

2 participants