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

refactor: generated instrumented client code part 2 #5398

Merged
merged 23 commits into from
Nov 18, 2022

Conversation

eddycharly
Copy link
Member

Explanation

This PR refactors generated instrumented client code.
It adds a tracing aware wrapper.

eddycharly and others added 20 commits November 16, 2022 11:41
Signed-off-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>
Signed-off-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>
Signed-off-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>
Signed-off-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>
Signed-off-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>
Signed-off-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>
Signed-off-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>
Signed-off-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>
Signed-off-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>
Signed-off-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>
Signed-off-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>
Signed-off-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>
Signed-off-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>
Signed-off-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>
Signed-off-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>
Signed-off-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>
Signed-off-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>
@codecov
Copy link

codecov bot commented Nov 17, 2022

Codecov Report

Merging #5398 (bef6797) into main (9983e82) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #5398   +/-   ##
=======================================
  Coverage   36.30%   36.30%           
=======================================
  Files         170      170           
  Lines       19072    19072           
=======================================
  Hits         6924     6924           
  Misses      11356    11356           
  Partials      792      792           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@realshuting realshuting left a comment

Choose a reason for hiding this comment

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

When do we need to update this instrumented code? And how?

Comment on lines +238 to +240
{{- $metricsPkg := Pkg "github.com/kyverno/kyverno/pkg/metrics" }}
{{- $discoveryPkg := Pkg "k8s.io/client-go/discovery" }}
{{- $restPkg := Pkg "k8s.io/client-go/rest" }}
Copy link
Member

Choose a reason for hiding this comment

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

Do we have restricted dependency on these folders? What do we need to keep in mind if we restructure the repo layout?

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you mean by restricted dependency ?
I wanted to avoid magic names so I added a func to resolve the import alias from a package path.
If the package path changes we will need to update them here.

Copy link
Member

Choose a reason for hiding this comment

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

Let's say I update the client code on my fork repo, will it look for github.com/kyverno/kyverno/pkg/.. or github.com/<forked-repo>/kyverno/pkg/..?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think so, this is a local package as per your go.mod

Copy link
Member

Choose a reason for hiding this comment

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

So it will look for the forked path?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, why not ?
The go.mod file in the forked repo says:

module github.com/kyverno/kyverno

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, wanted to confirm this.

@eddycharly
Copy link
Member Author

When do we need to update this instrumented code? And how?

make codegen-client-wrappers
I will add a verification step in our CI too.

@realshuting realshuting enabled auto-merge (squash) November 18, 2022 09:35
@realshuting realshuting merged commit 1b307ff into kyverno:main Nov 18, 2022
@eddycharly eddycharly deleted the instrumented-client-v2 branch November 18, 2022 10:07
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

2 participants