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

Upgrade version of oapi-codegen #7493

Closed
wants to merge 4 commits into from

Conversation

jamietanna
Copy link

@jamietanna jamietanna commented Feb 21, 2024

To get the latest features and bug fixes, we should bump to the latest
version of the library.

Thanks for using oapi-codegen!

Note that I've not updated

// 1. Use the latest version kin-openapi (can switch back when oapi-codegen will be updated)
- I'd recommend y'all as the maintainers have a look and see if there's anything that can be tweaked

May also be worth reviewing the upstream templates vs our versions here, in case there's anything else useful to bring in changes wise

To get the latest features and bug fixes, we should bump to the latest
version of the library.
@CLAassistant
Copy link

CLAassistant commented Feb 21, 2024

CLA assistant check
All committers have signed the CLA.

@jamietanna jamietanna marked this pull request as ready for review February 21, 2024 10:46
@arielshaqed
Copy link
Contributor

Hi @jamietanna ,
Sorry we dropped this for so long. I imagine that you can appreciate that any change like is equally scary and necessary. Let's start by getting all the tests to pass. I hope not to see many difficulties. A word of advance notice: our integration test "Esti", in particular, needs secrets, and I may copy this branch over to this repository to that we can do this.

@arielshaqed arielshaqed added infrastructure build, deploy and release processes include-changelog PR description should be included in next release changelog minor-change Used for PRs that don't require issue attached labels Mar 6, 2024
@arielshaqed
Copy link
Contributor

This looks really neat and a good idea! @jamietanna : did you manage to run make build test? This is roughly what this test tries to do and fails -- it looks like some dependency is missing, but after I run go get github.com/oapi-codegen/v2/runtime I get other breakages in building the generated code. (I pushed it on branch chore/oapi here, so you can just see it in 58a413a.)

Perhaps we're generating the files incorrectly? Or some further template updates are missing?

@arielshaqed
Copy link
Contributor

The new code has stricter typing for enums, which is cool! I fixed the relevant compilation breakage in 4d335e3, but now this func is broken.

This version bump is turning into a somewhat larger fix than I expected. I'd of course be happily grateful for your time to advise us on doing it -- but will you have time for us?

Again: thanks!

@jamietanna
Copy link
Author

Thanks for your changes to help with this too - I think these latest changes should pass - I'm just running through the tests locally now

@jamietanna
Copy link
Author

Oh hmm I see #7532

@jamietanna
Copy link
Author

jamietanna commented Mar 6, 2024

OK, so my confusion with getting the changes appears to be because we've got a mix of files that are and aren't committed - think I've managed to get there now (with many more build errors!)

@jamietanna
Copy link
Author

#7533 will be a prerequisite, then it'll be easier to see the difference when doing the upgrade 👍

@arielshaqed
Copy link
Contributor

#7533 will be a prerequisite, then it'll be easier to see the difference when doing the upgrade 👍

Currently we use make gen-api for generating these files; I do understand that this will not show you the diffs as part of git diff or similar.

@arielshaqed
Copy link
Contributor

The current compilation failures look like this. I'll try to fix them.

@arielshaqed
Copy link
Contributor

I kept on going over on my fork of your fork :-/ . But I hit a wall. I would appreciate your help getting 92e7e0e to compile.

The generated code has this method stub in the interface:

	// Abort a presign multipart upload
	// (DELETE /repositories/{repository}/branches/{branch}/staging/pmpu/{uploadId})
	AbortPresignMultipartUpload(w http.ResponseWriter, r *http.Request, repository string, branch string, uploadId string, params AbortPresignMultipartUploadParams)

This is the OpenAPI spec:

    delete:
      tags:
        - experimental
      operationId: abortPresignMultipartUpload
      summary: Abort a presign multipart upload
      description: Aborts a presign multipart upload.
      requestBody:
        content:
          application/json:
            schema:
              $ref: "#/components/schemas/AbortPresignMultipartUpload"
      responses:
        204:
          description: Presign multipart upload aborted
        400:
          $ref: "#/components/responses/BadRequest"
        401:
          $ref: "#/components/responses/Unauthorized"
        404:
          $ref: "#/components/responses/NotFound"
        420:
          description: too many requests
        default:
          $ref: "#/components/responses/ServerError"

As you may see, it takes a requestBody of type AbortPresignMultipartUpload. I don't understand: how does the generated server code deliver this body?

If it helps, here's the generated lakefs.gen.go.gz.

@arielshaqed
Copy link
Contributor

In fact I found this comment on a related OpenAPI issue. It seems that the OpenAPI 3.1 spec may have pulled the rug from beneath our feet.

This is a very real concern! We have a stable 1.0 API. Obviously we cannot change it to remove a DELETE method without bumping a new major version. Frankly it will not be worth the product effort to do this.

A possible mitigation is that this is an experimental API, which explicitly means we are allowed to change it. Opened #7538 to do something about this. But I would VERY MUCH like to keep going on this PR. @jamietanna , can you help us convince the codegen to relax this enforcement? I want to resolve this issue before the API leaves experimental, I want this PR, but I don't want this PR to be blocked by that issue. Realistically, we know that we have users who use this API. So while I can change this API at will, it will take at least 2 releases with a fairly long period to allow users to adapt between them.

@arielshaqed arielshaqed self-requested a review March 7, 2024 13:50
Copy link

github-actions bot commented Apr 7, 2024

This PR is now marked as stale after 30 days of inactivity, and will be closed soon. To keep it, mark it with the "no stale" label.

@github-actions github-actions bot added the stale label Apr 7, 2024
@arielshaqed
Copy link
Contributor

Thanks for the heads-up and help, @jamietanna! I'm continuing this on #7532. It's now compiling and everything.

@github-actions github-actions bot removed the stale label Apr 11, 2024
Copy link

This PR is now marked as stale after 30 days of inactivity, and will be closed soon. To keep it, mark it with the "no stale" label.

@github-actions github-actions bot added the stale label May 11, 2024
Copy link

Closing this PR because it has been stale for 7 days with no activity.

@github-actions github-actions bot closed this May 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
include-changelog PR description should be included in next release changelog infrastructure build, deploy and release processes minor-change Used for PRs that don't require issue attached stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants