Simplify the golang dependencies #77
Conversation
openshift-ci
bot
added
the
approved
There was a problem hiding this comment.
I'm OK with this direction in general. It's a (very very minimal) reimplementation of vendoring.
Can we:
- isolate the copied deps in their own subtree? (
pkg-k8s?imported? ) bothcpusetand thepodresourcesminimal client should sit in their own package (pkg-k8s/cpuset,pkg-k8s/podresources?) - clearly record (
pkg-k8s/README.md? comment at the beginning of each file) which commit are we importing?
pkg/k8s_imported/cpuset.go
Outdated
| @@ -4,6 +4,13 @@ Copyright 2017 The Kubernetes Authors. | |||
| This was copied out of kubernetes source to avoid pulling too | |||
| many dependencies with the full kubernetes source code. | |||
|
|
|||
| This is based on the following kubernetes file: | |||
|
|
|||
| https://github.com/kubernetes/kubernetes/blob/release-1.26/pkg/kubelet/cm/cpuset/cpuset.go | |||
There was a problem hiding this comment.
please comment (GH or better here) which sha1 was used (release-1.26 will always point to the tip of that branch)
openshift-ci
bot
added
do-not-merge/hold
There was a problem hiding this comment.
Content wise looks good. Left a couple of comments (/questions).
In general, I think we can do better in organizing the commits here. Also, we should include the links to source in the commit messages.
I would suggest one of the following:
- Squash the two commits and add the links to the source code in the commit message in addition to reference in code comment (like we have currently).
- Split it into two separate commits one for cpuset and another one for podresources and add the corresponding links to original source code in the receptive commit message.
I tend to prefer the latter as it would provide more granularity.
pkg/knit/cmd/k8s/podres.go
Outdated
| // GetV1Client returns a client for the PodResourcesLister grpc service | ||
| // This was copied from kubelet sources as per the comment there: | ||
| // | ||
| // Quote: |
There was a problem hiding this comment.
Can you add a reference to this quote?
pkg/knit/cmd/k8s/podres.go
Outdated
| } | ||
|
|
||
| // GetAddressAndDialer returns the address parsed from the given endpoint and a context dialer. | ||
| func GetAddressAndDialer(endpoint string) (string, func(ctx context.Context, addr string) (net.Conn, error), error) { |
There was a problem hiding this comment.
Why do we need this to be public? In case of kubelet source code this function was in util package so made sense to have it public but since in our case the consumer (GetV1Client) is in the same package making it private would make more sense to me.
This removes the dependency on kubernetes/kubernetes repository and replaces it with: - bundled cpuset from https://github.com/kubernetes/kubernetes/blob/759e043136b249f9e19355d3b2c9261b3ac82a70/pkg/kubelet/cm/cpuset/cpuset.go - bundled pod resources api client from https://github.com/kubernetes/kubernetes/blob/52457842d155743f0e3fc57ade87251cca37d375/pkg/kubelet/apis/podresources/client.go#L56 - kubernetes/kubelet (APIs only) The bundled code is placed into the pkg/k8s_imported directory to make it obvious where it is coming from. The effect is a significant reduction of indirect dependencies allowing easier consumption by other projects (like must gather). Signed-off-by: Martin Sivak <[email protected]>
|
@swatisehgal I decided to squash it into a single commit, because the justification is the same. I added both links to the original source code to the commit message though. |
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
|
@ffromani @swatisehgal Thanks for the reviews, lgtm would be nice :) |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: MarSik, swatisehgal 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 |
This removes the dependency on kubernetes/kubernetes repository and replaces it with: