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

New generic API client for less code duplication #56

Merged
merged 2 commits into from Nov 1, 2021

Conversation

LittleFox94
Copy link
Contributor

Description

This PR adds a new generic API client only requiring a handful lines of code on an API resource to be compatible to it. It's inspired by the kubernetes client, but not completely following it.

Many people reviewing this would be appreciated, while it seems to work with the already implemented API resources, I'm not sure if it's generic enough for all our APIs.

Feedback on the usage interface of this new client is also appreciated, I have uploaded the docs for this branch to the webserver to my home router (it's a wget -nH -m $godoc_server and some nginx redirects).

Release Note

Release note for CHANGELOG:

* adding a new generic API client, hopefully leading to way less code duplication in this package. The existing interfaces are still fully supported and not (yet, might happen in the future) deprecated.

References

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

@LittleFox94
Copy link
Contributor Author

Mirroring comment from @toothstone in internal ticket:

It might be a good idea to then upgrade one of the more challenging packages (e.g. vsphere) first, to really test out the limits and adapt where necessary.

This might even be still in scope of this PR

Copy link
Contributor

@kstiehl kstiehl 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 looked at the pagination package yet

But here are some of my findings
In general i like the refactoring just a few things

also please adjust all errors so that they are not capitalized anymore.

pkg/api/errors.go Outdated Show resolved Hide resolved
pkg/api/errors.go Outdated Show resolved Hide resolved
pkg/api/api_implementation.go Outdated Show resolved Hide resolved
pkg/api/api_implementation.go Outdated Show resolved Hide resolved
pkg/api/api_implementation.go Outdated Show resolved Hide resolved
pkg/api/api_implementation.go Outdated Show resolved Hide resolved
@LittleFox94 LittleFox94 force-pushed the syseng-616/feature/new-client-api branch 4 times, most recently from 2af1f33 to 23af55e Compare October 21, 2021 14:58
@LittleFox94
Copy link
Contributor Author

I think I addressed all your review comments and fixed them in one way or another.

I also implemented filters on listing LBaaS backends as example (and for my tests). Oh right, I made all the examples to working, runnable tests - 73.5% coverage from only the examples already :)
Will add more tests, but another review already would be appreciated.

@kstiehl
Copy link
Contributor

kstiehl commented Oct 21, 2021

Sure will have another look

@LittleFox94 LittleFox94 force-pushed the syseng-616/feature/new-client-api branch 5 times, most recently from f3b30b6 to eef0226 Compare October 27, 2021 14:18
@anexia-it anexia-it deleted a comment from codecov-commenter Oct 27, 2021
@LittleFox94 LittleFox94 force-pushed the syseng-616/feature/new-client-api branch from eef0226 to aacb872 Compare October 27, 2021 14:25
@codecov-commenter
Copy link

Codecov Report

Merging #56 (aacb872) into main (815fcca) will increase coverage by 8.48%.
The diff coverage is 93.89%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #56      +/-   ##
==========================================
+ Coverage   45.40%   53.88%   +8.48%     
==========================================
  Files          76       82       +6     
  Lines        2938     3511     +573     
==========================================
+ Hits         1334     1892     +558     
- Misses       1271     1286      +15     
  Partials      333      333              
Flag Coverage Δ
integration 45.13% <0.00%> (-1.10%) ⬇️
unittests 20.79% <93.89%> (+14.93%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/lbaas/frontend/api.go 13.33% <0.00%> (-86.67%) ⬇️
pkg/lbaas/loadbalancer/api.go 35.71% <25.00%> (-64.29%) ⬇️
pkg/lbaas/backend/api.go 89.28% <88.46%> (-10.72%) ⬇️
pkg/api/api_implementation.go 96.46% <96.46%> (ø)
pkg/api/errors.go 100.00% <100.00%> (ø)
pkg/api/internal/options.go 100.00% <100.00%> (ø)
pkg/api/options.go 100.00% <100.00%> (ø)
pkg/api/pagination.go 100.00% <100.00%> (ø)
pkg/api/types/options.go 100.00% <100.00%> (ø)
pkg/client/log_utils.go 89.15% <0.00%> (+8.43%) ⬆️
... and 1 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 815fcca...aacb872. Read the comment docs.

@LittleFox94 LittleFox94 force-pushed the syseng-616/feature/new-client-api branch 2 times, most recently from be4c3c4 to 7985962 Compare October 29, 2021 07:48
pkg/api/api_implementation.go Outdated Show resolved Hide resolved
pkg/api/api_implementation.go Outdated Show resolved Hide resolved
Adding a new generic API client which needs only a few lines of code
per resource type to be usable with it.

Gitmoji-marked as Refactoring and New Feature since it is a new feature
allowing us to refactor everything and reduce code duplication.
This commit implements the new API client interfaces on LBaaS
loadbalancer, backend and frontend.
@LittleFox94 LittleFox94 force-pushed the syseng-616/feature/new-client-api branch from 7985962 to 0112f7e Compare October 30, 2021 22:01
@LittleFox94 LittleFox94 merged commit 5ebe629 into main Nov 1, 2021
@LittleFox94 LittleFox94 deleted the syseng-616/feature/new-client-api branch November 1, 2021 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants