refactor: simplify ClientSet and add comments #700
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/S
|
/assign @cheftako |
| return ok | ||
| } | ||
|
|
||
| // HasID returns true if the ClientSet has a client to the specified serverID. | ||
| func (cs *ClientSet) HasID(serverID string) bool { |
There was a problem hiding this comment.
HasID looks unused, can be trimmed?
There was a problem hiding this comment.
So because it is exported, I wasn't sure that it was safe to remove - it could be used from another project maybe? But maybe it's only konnectivity-client that we consider part of our "API contract"?
Do we run an e2e where we just check that e.g. kube-apiserver still builds - I think that's probably our primary client, and I think I'd be very comfortable removing exported functions as long as apiserver still compiles.
On the other hand, I do agree, it's probably safe to remove, but maybe lots of small safe cleanups are better than one big (and potentially risky) cleanup?
There was a problem hiding this comment.
So because it is exported, I wasn't sure that it was safe to remove - it could be used from another project maybe? But maybe it's only
konnectivity-clientthat we consider part of our "API contract"?
Your point is right, but historically pkg/agent has always been specific to konnectivity-agent binary.
Do we run an e2e where we just check that e.g. kube-apiserver still builds - I think that's probably our primary client, and I think I'd be very comfortable removing exported functions as long as apiserver still compiles.
apiserver doesn't depend on pkg/agent. This repo has .github/workflows/e2e.yaml checking apiserver for k8s versions against locally-built konnectivity-server and konnectivity-agent, but mistakes made in konnectivity-client aren't fully detected until attempts to release to k/k.
On the other hand, I do agree, it's probably safe to remove, but maybe lots of small safe cleanups are better than one big (and potentially risky) cleanup?
I would still include it, but no objections to keeping as-is, this PR is strictly improvement.
|
/lgtm /hold HOLD only for the optional nit and allow @cheftako to look. |
k8s-ci-robot
added
the
do-not-merge/hold
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jkh52, justinsb 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 |
k8s-ci-robot
added
the
approved
No description provided.