-
Notifications
You must be signed in to change notification settings - Fork 298
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
feat: add token requests client #805
Conversation
Skipping CI for Draft Pull Request. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aramase 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 |
e9116d9
to
f97a8ad
Compare
f9c5e53
to
7b341eb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great, gonna need a bit more time to think a few things through though!
sigs.k8s.io/yaml v1.3.0 // indirect | ||
) | ||
|
||
replace ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i feel like there must be a way to do this without all these replace directives (I'm goin got checkout the branch and try playing around with some things to make go mod
happy...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay so I think i understand this a bit better...
This PR introduces a dependency on https://pkg.go.dev/k8s.io/kubernetes/pkg/kubelet/token
which pulls in the k8s.io/kubernetes v1.23.0
module, but according to kubernetes/kubernetes#90358 (comment)
"depending on k8s.io/kubernetes directly as a library, which is not supported"
I guess what we're doing here is whats mentioned kubernetes/kubernetes#79384
Since this token dependency is not permanent (we have plan to remove and replace with RequiresRepublish
once thats supposed by all K8s supported releases) I think I'm good with these replace
directives - but we should definitely add a comment explaining that k8s.io/kubernetes
dependency causes the problems linking to kubernetes/kubernetes#79384
/label tide/merge-method-squash |
b3a33b8
to
04970bc
Compare
/test pull-secrets-store-csi-driver-e2e-azure |
2f66ddc
to
7d53974
Compare
gopkg.in/check.v1 v1.0.0-20200227125254-8fa46927fb4f // indirect | ||
gopkg.in/yaml.v2 v2.4.0 // indirect | ||
) | ||
|
||
replace ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oof i'm noticing that this doesnt directly depend on k8s.io/kubernetes
but still needs all these replace directives...
I wonder if providers (which depend on sigs.k8s.io/secrets-store-csi-driver
to pull in the protobuf interface) will also need the replace directives. If so we may wanna move the protobuf to its own go module so it doesnt pull in all the transitive dependency issues.
(we dont need to do that as part of this PR)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
opened an issue for it: #852
Looking good! just a few more comments |
Signed-off-by: Anish Ramasekar <[email protected]>
Signed-off-by: Anish Ramasekar <[email protected]>
Signed-off-by: Anish Ramasekar <[email protected]>
Signed-off-by: Anish Ramasekar <[email protected]>
Signed-off-by: Anish Ramasekar <[email protected]>
/lgtm |
Signed-off-by: Anish Ramasekar [email protected]
What this PR does / why we need it:
NodeUnpublishVolume
.Which issue(s) this PR fixes (optional, using
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when the PR gets merged):Fixes #
Special notes for your reviewer: