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

Added OpenAPI Codegen #474

Merged
merged 11 commits into from
Oct 9, 2024
Merged

Added OpenAPI Codegen #474

merged 11 commits into from
Oct 9, 2024

Conversation

gehrkefc
Copy link
Contributor

@gehrkefc gehrkefc commented Sep 24, 2024

This is related to issue: 47007

@gehrkefc gehrkefc requested a review from a team as a code owner September 24, 2024 18:52
@gehrkefc gehrkefc changed the title added openapi codegen Added OpenAPI Codegen Sep 26, 2024
pkg/controller-gen/main.go Outdated Show resolved Hide resolved
pkg/controller-gen/main.go Show resolved Hide resolved
pkg/controller-gen/main.go Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
pkg/controller-gen/main.go Outdated Show resolved Hide resolved
@gehrkefc gehrkefc requested a review from tomleb September 30, 2024 18:56
Copy link
Contributor

@tomleb tomleb left a comment

Choose a reason for hiding this comment

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

2 more comments but otherwise looks good and worked fine in rancher/rancher to generate OpenAPI documents of types.

go.mod Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
@gehrkefc
Copy link
Contributor Author

gehrkefc commented Oct 2, 2024

@tomleb You were right, I worked to adjust both gengo and kube-openapi to compatible version and it worked! Thank you!

tomleb
tomleb previously approved these changes Oct 2, 2024
Copy link

@maxsokolovsky maxsokolovsky left a comment

Choose a reason for hiding this comment

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

Looks good! Had just a couple of very small questions...

pkg/controller-gen/main.go Outdated Show resolved Hide resolved
pkg/controller-gen/main.go Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
@@ -95,6 +95,7 @@ func (cg *ClientGenerator) typesGroupVersionDocPackage(name *types.Name, gv sche
p.HeaderComment = []byte(fmt.Sprintf(`
%s

// +k8s:openapi-gen=true
Copy link
Contributor

Choose a reason for hiding this comment

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

We should make this dependent on the GenerateOpenAPI flag.

I thought we were okay with just doing the same as deepcopy but I misunderstood that deepcopy is generated when this generator runs..

It will also prevent having a huge amount of file changes in rancher/rancher when we update wrangler. (And also will prevent any change in behavior due to that new directive)

Could you make that change? Sorry about this.

@tomleb tomleb self-requested a review October 3, 2024 14:57
@tomleb tomleb dismissed their stale review October 3, 2024 14:58

changes needed

Copy link
Contributor

@ericpromislow ericpromislow left a comment

Choose a reason for hiding this comment

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

Just a couple of cosmetic changes related to errors

Copy link
Contributor

@ericpromislow ericpromislow left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@tomleb tomleb left a comment

Choose a reason for hiding this comment

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

Tested it in r/r and code with the flag off don't modify current generated code and when the flag is on it does generate properly.

@gehrkefc gehrkefc merged commit 1345a30 into master Oct 9, 2024
4 checks passed
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.

4 participants