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

Use Connection() from util package #234

Merged
merged 2 commits into from
Mar 1, 2019

Conversation

jsafrane
Copy link
Contributor

The result should be the same as in kubernetes-csi/external-attacher#123

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 22, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsafrane

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 22, 2019
@jsafrane jsafrane force-pushed the use-lib-connection branch 2 times, most recently from 41d9159 to a9543a0 Compare February 22, 2019 17:51
return "", fmt.Errorf("name is empty")
}
return name, nil
return connection.GetDriverName(ctx, conn)
}

func getDriverCapabilities(conn *grpc.ClientConn, timeout time.Duration) (sets.Int, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

and update getcapabilities?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

@msau42
Copy link
Collaborator

msau42 commented Feb 25, 2019

cc @lpabon

}
switch service.GetType() {
for cap := range pluginCaps {
switch cap {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is mixing plugin and controller capabilities into the same set correct? Could you have conflicts in values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PluginCapability_* and ControllerCapability_* are part of the same enum defined in the provisioner, they don't come from CSI:

const (
PluginCapability_CONTROLLER_SERVICE = iota
PluginCapability_ACCESSIBILITY_CONSTRAINTS
ControllerCapability_CREATE_DELETE_VOLUME
ControllerCapability_CREATE_DELETE_SNAPSHOT
)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for pointing that out. Is there a reason why it needs to have one more level of translation from CSI capabilities?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we sort it out in a separate PR? I'd like to cache the capabilities so provisioner does not load them from the driver every time it needs them. Refactoring of the enum would be better there.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure do you want to use csi-lib-utils 0.4.0-rc1 here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to 0.4.0-rc1

@jsafrane jsafrane force-pushed the use-lib-connection branch from 2aeb480 to 20cb5e5 Compare March 1, 2019 15:09
Copy link
Collaborator

@msau42 msau42 left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 1, 2019
@k8s-ci-robot k8s-ci-robot merged commit f588078 into kubernetes-csi:master Mar 1, 2019
humblec added a commit to humblec/external-provisioner that referenced this pull request Sep 8, 2023
984feece4 Merge pull request kubernetes-csi#234 from siddhikhapare/csi-tools
1f7e60599 fixed broken links of testgrid dashboard

git-subtree-dir: release-tools
git-subtree-split: 984feece4bafac3aad74deeed76a500a0c485fb1
kbsonlong pushed a commit to kbsonlong/external-provisioner that referenced this pull request Dec 29, 2023
humblec added a commit that referenced this pull request Mar 6, 2024
984feece4 Merge pull request #234 from siddhikhapare/csi-tools
1f7e60599 fixed broken links of testgrid dashboard

git-subtree-dir: release-tools
git-subtree-split: 984feece4bafac3aad74deeed76a500a0c485fb1
tyuchn added a commit to tyuchn/external-provisioner that referenced this pull request Mar 19, 2024
dc4d0ae20 Merge pull request kubernetes-csi#249 from jsafrane/use-go-version
e681b170e Use .go-version to get Kubernetes go version
b54c1ba49 Merge pull request kubernetes-csi#246 from xing-yang/go_1.21
5436c81e9 Change go version to 1.21.5
267b40e97 Merge pull request kubernetes-csi#244 from carlory/sig-storage
b42e5a2de nominate self (carlory) as kubernetes-csi reviewer
a17f536fc Merge pull request kubernetes-csi#210 from sunnylovestiramisu/sidecar
011033de2 Use set -x instead of die
5deaf667c Add wrapper script for sidecar release
f8c8cc4c7 Merge pull request kubernetes-csi#237 from msau42/prow
b36b5bfdc Merge pull request kubernetes-csi#240 from dannawang0221/upgrade-go-version
adfddcc9a Merge pull request kubernetes-csi#243 from pohly/git-subtree-pull-fix
c4650889d pull-test.sh: avoid "git subtree pull" error
7b175a1e2 Update csi-test version to v5.2.0
987c90ccd Update go version to 1.21 to match k/k
2c625d41d Add script to generate patch release notes
f9d5b9c05 Merge pull request kubernetes-csi#236 from mowangdk/feature/bump_csi-driver-host-path_version
b01fd5372 Bump csi-driver-host-path version up to v1.12.0
984feece4 Merge pull request kubernetes-csi#234 from siddhikhapare/csi-tools
1f7e60599 fixed broken links of testgrid dashboard

git-subtree-dir: release-tools
git-subtree-split: dc4d0ae20a3dcce17fbfc745fb1f1e3b10cd9644
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants