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.

Sign up for GitHub

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

Jump to bottom

Generate go-client from openapi #4490

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Generate go-client from openapi #4490

wants to merge 2 commits into from

Conversation

xoxys
Copy link
Member

@xoxys xoxys commented Nov 30, 2024 •
edited by qwerty287
Loading

Supersedes: #2691
closes #3053

  • fix tests
  • add interface and mocks
  • drop stream.go: this functionality does not exist in the old client anyway and can be implemented in a dedicated PR

@6543
Copy link
Member

6543 commented Nov 30, 2024

https://ci.woodpecker-ci.org/repos/3780/pipeline/23454/42

@xoxys xoxys force-pushed the gen-woodpecker-go branch from 28ad1fa to b692932 Compare November 30, 2024 16:22
@xoxys
Copy link
Member Author

xoxys commented Nov 30, 2024

@anbraten I had to drop the entire history due to the pretty old merge commits, and I was not able to do a proper rebase to main. Sorry about this, if you are not ok with it please let me know!

@xoxys
Copy link
Member Author

xoxys commented Nov 30, 2024

https://ci.woodpecker-ci.org/repos/3780/pipeline/23454/42

Yes, I'm aware of it. I would like to silence it for now and do the migration to the new go client in a dedicated PR? What do you think?

@6543
Copy link
Member

6543 commented Nov 30, 2024

sure just add a rule to the golint file ;)

@xoxys xoxys force-pushed the gen-woodpecker-go branch from 59c3c51 to ad4f353 Compare November 30, 2024 16:38
6543
6543 reviewed Nov 30, 2024
View reviewed changes
.mockery.yaml Show resolved Hide resolved
@qwerty287 qwerty287 added the breaking will break existing installations if no manual action happens label Nov 30, 2024
@xoxys xoxys force-pushed the gen-woodpecker-go branch from 5ab795b to e79ec92 Compare November 30, 2024 19:51
@xoxys
Copy link
Member Author

xoxys commented Nov 30, 2024

@qwerty287 This PR is not really breaking, is it? It does not remove the legacy client.

@xoxys xoxys added refactor delete or replace old code lib about our woodpecker-go api lib labels Nov 30, 2024
@xoxys
Copy link
Member Author

xoxys commented Nov 30, 2024

I start to share the opinion from @lafriks #3053 (comment)

The autogenerated sdk is really pain to use... No idea if we can improve this somehow, otherwise I also tend to stick to a handcrafted sdk.

@xoxys xoxys marked this pull request as draft November 30, 2024 21:06
@anbraten
Copy link
Member

anbraten commented Nov 30, 2024 •
edited
Loading

Had the same feeling while using this current approach as well. Mainly the syntax seems weird to me in general generated libs can be quite awesome to maintain and use. At least the generated types are quite nice already.

@xoxys xoxys changed the title Generate go-client from swagger Generate go-client from openapi Nov 30, 2024
@xoxys
Copy link
Member Author

xoxys commented Nov 30, 2024

Yes the types are nice but also have some issues e.g.

	StepId *int          `json:"step_id,omitempty"`

should be StepID or stepID and I dont see a way to control this behavior. So do we just accept it?

@xoxys
Copy link
Member Author

xoxys commented Nov 30, 2024 •
edited
Loading

Looks like it can even lead to clashes... oapi-codegen/oapi-codegen#452 All in all Im not convinced. I think Ill stick to our maintained sdk.

@6543
Copy link
Member

6543 commented Nov 30, 2024 •
edited
Loading

This is not the only lib to gen code ... If it has to many downsides, we might look for another lib that does the same ...
... in the worst case we should automate wat we do manually now ourselve

@qwerty287 qwerty287 removed the breaking will break existing installations if no manual action happens label Dec 1, 2024
@pat-s
Copy link
Contributor

pat-s commented Dec 1, 2024

So, #2691 can be closed then and removed from the 3.0 milestone?

@xoxys
Copy link
Member Author

xoxys commented Dec 1, 2024

At least in my opinion its unlikely to be part of v3 and the release should not be blocked by it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@6543 6543 6543 left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
lib about our woodpecker-go api lib refactor delete or replace old code
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Generate sdk from swagger
5 participants
@xoxys @6543 @anbraten @pat-s @qwerty287