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

Jump to bottom

Decouple REST Framework #252

Merged
merged 6 commits into from Jan 12, 2022
Merged

Decouple REST Framework #252

merged 6 commits into from Jan 12, 2022

Conversation

austince
Copy link
Contributor

@austince austince commented Oct 7, 2021 •
edited

As discussed in the #250, the go-restful dependency is currently exposed in a few places, making it difficult to use kube-openapi in projects using other frameworks (or versions of go-restful). This may make it easier to eventually move away from go-restful in k/k as well 🤷🏼.

This PR defines interfaces based on the go-restful types and exposes them instead. It also creates an adapter package to make it easy to use the current go-restful version with the new interfaces.

The interfaces are close to go-restful, but:

No breaking changes are introduced, though all places that expose the restful dependency instead of the new interfaces are marked as deprecated.

The PR also adds some small changes to integration tests/docs so its easier to develop. Can move that out of this PR if desired.

/cc @jpbetz @deads2k

Closes #250

@k8s-ci-robot k8s-ci-robot added do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 7, 2021
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Oct 7, 2021
@deads2k
Copy link
Contributor

deads2k commented Oct 8, 2021

/ok-to-test

@k8s-ci-robot k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Oct 8, 2021
@deads2k
Copy link
Contributor

deads2k commented Oct 8, 2021

/ok-to-test

huh, I guess it's now this approve and run button.

@deads2k
Copy link
Contributor

deads2k commented Oct 8, 2021

The approach and the goal seems reasonable to me. I'll defer the detailed review to someone closer to the library, but I'd be very happy to lose the gorestful dependency.

@austince
Copy link
Contributor Author

austince commented Nov 8, 2021

(I haven't abandoned this PR, just waiting for #247 to land since it is the priority and will have quite a few conflicts. :) )

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 12, 2021
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 2, 2022
@austince
Copy link
Contributor Author

austince commented Jan 3, 2022

/assign @sttts
/cc @jpbetz

This current PR includes breaking changes. A couple months ago we talked about doing proper semver in this repo – wdyt, is that worth the effort? Otherwise, we could hide the breaking changes at the cost of making the BuildOpenAPISpec + config GetOperationIDAndTags funcs take an empty interface instead and do type switching..

@jpbetz
Copy link
Contributor

jpbetz commented Jan 4, 2022

/assign Dr. Stefan Schimanski /cc Joe Betz

This current PR includes breaking changes. A couple months ago we talked about doing proper semver in this repo – wdyt, is that worth the effort? Otherwise, we could hide the breaking changes at the cost of making the BuildOpenAPISpec + config GetOperationIDAndTags funcs take an empty interface instead and do type switching..

I'm generally in favor of not breaking backward compatibility unless we really need to. I think we can get by here without breaking changes.

How about we introduce a new BuildOpenAPISpecFromRoutes function and mark BuildOpenAPISpec as deprecated? If it's possible to have BuildOpenAPISpec delegate most of the implementation to BuildOpenAPISpecFromRoutes that would also be great, but at a quick glance that might not be easy to do here.

Same idea applies for GetOperationIDAndTags. Can we introduce a new function and deprecate the old one?

@austince
Copy link
Contributor Author

austince commented Jan 4, 2022

+1, thanks for the review & seems reasonable to me! The only side effect is that go-restful will be a dep even if unused. I'll make those changes by EOW.

@austince austince force-pushed the feat/decouple-restful branch 3 times, most recently from 071a9a0 to db2d21f Compare January 5, 2022 18:11
@austince
Decouple REST Framework
c4d1aeb 
Takes ownership of the route interfaces
and removes exposing the `go-restful`
dependency.

Adds an adapter for the currently used
version of `go-restful`.
@austince austince force-pushed the feat/decouple-restful branch 2 times, most recently from 8321b59 to 4dc6e95 Compare January 5, 2022 18:29
@austince
Copy link
Contributor Author

austince commented Jan 5, 2022

@jpbetz I believe I've addressed those notes – all places that expose the restful dependency are deprecated and new funcs/fields are added with the new interfaces. Please have another look when you have a chance.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 5, 2022
@jpbetz
Copy link
Contributor

jpbetz commented Jan 5, 2022

/assign @sttts
Would you be willing to review for approval?

@austince austince mentioned this pull request Jan 7, 2022
Closed
@sttts
Copy link
Contributor

sttts commented Jan 10, 2022

Is there a proof PR in k/k ?

sttts
sttts reviewed Jan 10, 2022
View reviewed changes
pkg/builder3/util.go Outdated Show resolved Hide resolved
@sttts
Copy link
Contributor

sttts commented Jan 10, 2022

Sgtm. Waiting for a proof PR before approval.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 10, 2022
@austince
Decouple integretion tests from GOPATH
e9ebba8 
The header file is loaded from the GOPATH
k8s dir by default, which makes it impossible
to develop if you do not have the local
dependencies in your GOPATH.

This commit ensures the header file is
loaded from the project's directory.

It also includes a cleanup of the README
and bumps the timeout to 10s.

Signed-off-by: austin ce <austin.cawley@gmail.com>
austince added a commit to austince/kubernetes that referenced this pull request Jan 10, 2022
@austince
Use forked kube-openapi
cc8c22d 
Uses github.com/austince/kube-openapi v0.0.0-restframework-e9ebba8
in place of all k8s.io/kube-openapi.

Proof PR for kubernetes/kube-openapi#252

Signed-off-by: austin ce <austin.cawley@gmail.com>
@austince austince mentioned this pull request Jan 10, 2022
Closed
@austince
Copy link
Contributor Author

Sgtm. Waiting for a proof PR before approval.

Is this what you're after in a proof PR? kubernetes/kubernetes#107453

@sttts
Copy link
Contributor

sttts commented Jan 11, 2022

Yes. Applied labels to see test results.

@austince
Copy link
Contributor Author

Turned up one error it seems. Fixed, working on covering the case in tests in this repo.

@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 11, 2022
@austince
Copy link
Contributor Author

@sttts Proof PR tests are now all passing, please have another look. Thanks!

@sttts
Copy link
Contributor

sttts commented Jan 12, 2022

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 12, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: austince, sttts

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 12, 2022
@k8s-ci-robot k8s-ci-robot merged commit c1fc1de into kubernetes:master Jan 12, 2022
@austince austince deleted the feat/decouple-restful branch January 12, 2022 15:07
@austince
Copy link
Contributor Author

Thanks so much!

@austince austince mentioned this pull request May 9, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sttts sttts sttts left review comments

@deads2k deads2k deads2k left review comments

@jpbetz jpbetz Awaiting requested review from jpbetz

Assignees

@jpbetz jpbetz

@sttts sttts

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Decouple REST Framework
5 participants
@austince @deads2k @jpbetz @sttts @k8s-ci-robot