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

Feature/openapi3 #1667

Merged
merged 41 commits into from
Apr 1, 2021
Merged

Feature/openapi3 #1667

merged 41 commits into from
Apr 1, 2021

Conversation

nopcoder
Copy link
Contributor

@nopcoder nopcoder commented Mar 22, 2021

Using OpenAPI3 using https://github.com/deepmap/oapi-codegen

Modified the server template code to parse incoming body in case of one body in JSON format.
Removed the python code which verifies the schema, it is not compatible with v3.

TODOs

  • align the object put/get API to be compatible with the previous version
  • test must of lakectl commands with the previous version

@codecov-io
Copy link

codecov-io commented Mar 24, 2021

Codecov Report

Merging #1667 (5c5e2c1) into master (d5605ac) will increase coverage by 1.76%.
The diff coverage is 51.02%.

❗ Current head 5c5e2c1 differs from pull request most recent head 9db1f66. Consider uploading reports for the commit 9db1f66 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1667      +/-   ##
==========================================
+ Coverage   39.87%   41.64%   +1.76%     
==========================================
  Files         171      168       -3     
  Lines       13957    13315     -642     
==========================================
- Hits         5566     5545      -21     
+ Misses       7609     7029     -580     
+ Partials      782      741      -41     
Impacted Files Coverage Δ
pkg/actions/webhook.go 69.89% <0.00%> (ø)
pkg/api/controller.go 32.62% <ø> (-0.46%) ⬇️
pkg/api/login_handler.go 9.61% <0.00%> (ø)
pkg/auth/service.go 36.30% <0.00%> (ø)
pkg/db/connect.go 68.75% <ø> (ø)
pkg/db/logged_rows.go 0.00% <ø> (ø)
pkg/gateway/multiparts/tracker.go 88.57% <ø> (+88.57%) ⬆️
pkg/graveler/errors.go 80.00% <ø> (ø)
pkg/httputil/logging.go 0.00% <0.00%> (ø)
pkg/httputil/tracing.go 0.00% <0.00%> (ø)
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d5605ac...9db1f66. Read the comment docs.

Copy link
Contributor

@arielshaqed arielshaqed left a comment

Choose a reason for hiding this comment

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

Thanks!

Trying to grow this mega-PR. Just skimmed swagger.yml, mostly to show my ignorance. Happy for comments that will tell me what is most urgent for me to read first, the better to review this PR.

api/swagger.yml Outdated Show resolved Hide resolved
api/swagger.yml Show resolved Hide resolved
api/swagger.yml Outdated Show resolved Hide resolved
api/swagger.yml Outdated Show resolved Hide resolved
api/swagger.yml Show resolved Hide resolved
api/swagger.yml Show resolved Hide resolved
api/swagger.yml Outdated Show resolved Hide resolved
api/swagger.yml Show resolved Hide resolved
api/swagger.yml Show resolved Hide resolved
api/swagger.yml Show resolved Hide resolved
@nopcoder nopcoder marked this pull request as ready for review March 29, 2021 07:32
Copy link
Contributor

@itaiad200 itaiad200 left a comment

Choose a reason for hiding this comment

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

Haven't gone thru it all yet.. looking great so far

pkg/api/auth_middleware.go Outdated Show resolved Hide resolved
pkg/api/auth_middleware.go Outdated Show resolved Hide resolved
pkg/api/auth_middleware.go Outdated Show resolved Hide resolved
pkg/api/auth_middleware.go Outdated Show resolved Hide resolved
pkg/api/auth_middleware_test.go Outdated Show resolved Hide resolved
pkg/api/tmpl/chi-interface.tmpl Outdated Show resolved Hide resolved
pkg/api/tmpl/chi-middleware.tmpl Show resolved Hide resolved
pkg/api/tmpl/chi-middleware.tmpl Show resolved Hide resolved
pkg/api/controller.go Show resolved Hide resolved
pkg/api/controller.go Show resolved Hide resolved
@nopcoder nopcoder self-assigned this Mar 30, 2021
@nopcoder
Copy link
Contributor Author

Thanks!

Trying to grow this mega-PR. Just skimmed swagger.yml, mostly to show my ignorance. Happy for comments that will tell me what is most urgent for me to read first, the better to review this PR.

Merged the client upload-related changes - the wrapper code above the client API is gone so I've added it to the client's fs.go.

Copy link
Contributor

@arielshaqed arielshaqed left a comment

Choose a reason for hiding this comment

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

Thanks! Looks very nice.

Partial comments; will do the rest after rebase is pushed.

benchmarks/main.go Show resolved Hide resolved
cmd/lakectl/cmd/auth.go Show resolved Hide resolved
docs/assets/js/swagger.yml Show resolved Hide resolved
docs/assets/js/swagger.yml Outdated Show resolved Hide resolved
docs/assets/js/swagger.yml Outdated Show resolved Hide resolved
docs/assets/js/swagger.yml Outdated Show resolved Hide resolved
@nopcoder nopcoder requested a review from arielshaqed April 1, 2021 09:03
Copy link
Contributor

@arielshaqed arielshaqed left a comment

Choose a reason for hiding this comment

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

Neat, very nice!

Required changes:

  • Upload errors are handled unsafely in lakectl -- succeed on unknown error.

Additional (meta-) comments:

  • The "WithResponses" client interface is quite nasty. Maybe just not use it? (Not necessary in this PR)

benchmarks/main.go Show resolved Hide resolved
benchmarks/main.go Outdated Show resolved Hide resolved
benchmarks/main.go Outdated Show resolved Hide resolved
cmd/lakectl/cmd/auth.go Show resolved Hide resolved
cmd/lakectl/cmd/common_helpers.go Outdated Show resolved Hide resolved
pkg/api/auth_middleware.go Show resolved Hide resolved
pkg/api/auth_middleware.go Outdated Show resolved Hide resolved
pkg/api/auth_middleware.go Outdated Show resolved Hide resolved
// Find route
route, pathParams, err := router.FindRoute(r)
if err != nil {
return http.StatusBadRequest, err // We failed to find a matching route for the request.
Copy link
Contributor

Choose a reason for hiding this comment

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

REST says there is no such difference; I have to agree with @itaiad200 here. 400 is always appropriate (because it says so little), however accessing a URL that is not mounted is a classic 404.

pkg/api/transform.go Show resolved Hide resolved
@nopcoder nopcoder removed the request for review from itaiad200 April 1, 2021 14:03
@nopcoder nopcoder requested review from itaiad200 and arielshaqed and removed request for itaiad200 April 1, 2021 15:21
@nopcoder nopcoder dismissed itaiad200’s stale review April 1, 2021 15:23

applied requested changes

Copy link
Contributor

@arielshaqed arielshaqed left a comment

Choose a reason for hiding this comment

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

Great, thanks!

Please review whether we wnat to return 404 for an unknown path in validateRequest. Any which way, I think this is good (great..) to go!

api/swagger.yml Show resolved Hide resolved
cmd/lakectl/cmd/fs.go Show resolved Hide resolved
@nopcoder nopcoder merged commit 6a01ee3 into master Apr 1, 2021
@ozkatz
Copy link
Collaborator

ozkatz commented Apr 2, 2021

🎉

@einato-zz
Copy link
Contributor

einato-zz commented Apr 2, 2021 via email

@nopcoder nopcoder mentioned this pull request Apr 4, 2021
@nopcoder nopcoder deleted the feature/openapi3 branch April 12, 2021 11:19
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.

6 participants