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 external-attacher to use csi-lib-utils/rpc #127

Merged
merged 2 commits into from Mar 7, 2019

Conversation

@MorrisLaw
Copy link
Contributor

commented Mar 2, 2019

This PR makes use of the new csi-lib-utils/rpc package.

@k8s-ci-robot k8s-ci-robot requested review from msau42 and saad-ali Mar 2, 2019

@k8s-ci-robot k8s-ci-robot added the size/XL label Mar 2, 2019

@MorrisLaw

This comment has been minimized.

Copy link
Contributor Author

commented Mar 2, 2019

@MorrisLaw MorrisLaw changed the title Consistent sidecars refactor external-attacher to use csi-lib-utils/rpc Mar 2, 2019

@MorrisLaw MorrisLaw changed the title refactor external-attacher to use csi-lib-utils/rpc Refactor external-attacher to use csi-lib-utils/rpc Mar 2, 2019

@MorrisLaw MorrisLaw force-pushed the MorrisLaw:consistent-sidecars branch from 4ad0038 to 4f31d57 Mar 2, 2019

pkg/attacher/connection.go Outdated Show resolved Hide resolved
pkg/attacher/connection.go Outdated Show resolved Hide resolved
pkg/attacher/connection.go Outdated Show resolved Hide resolved
pkg/attacher/connection.go Outdated Show resolved Hide resolved
pkg/attacher/connection.go Outdated Show resolved Hide resolved
pkg/attacher/connection.go Outdated Show resolved Hide resolved
pkg/attacher/connection.go Outdated Show resolved Hide resolved
pkg/attacher/connection.go Outdated Show resolved Hide resolved
@jsafrane

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2019

There is something missing in this PR, I see it removes Probe and GetControllerCapabilities from connection.go, but it does not start using the ones from csi-lib-util.

pkg/connection/connection.go Outdated Show resolved Hide resolved
@andrewsykim

This comment has been minimized.

Copy link
Member

commented Mar 4, 2019

There is something missing in this PR, I see it removes Probe and GetControllerCapabilities from connection.go, but it does not start using the ones from csi-lib-util.

It was removed from the interface but it was still implemented by CSIConnection. @MorrisLaw is fixing that now so we just call Probe and GetControllerCapabilities directly from rpc package.

@andrewsykim andrewsykim added this to In progress in Consistent sidecars Mar 4, 2019

@MorrisLaw MorrisLaw force-pushed the MorrisLaw:consistent-sidecars branch from c79a812 to 721f0ad Mar 7, 2019

@MorrisLaw MorrisLaw force-pushed the MorrisLaw:consistent-sidecars branch from 721f0ad to 158f000 Mar 7, 2019

@andrewsykim

This comment has been minimized.

Copy link
Member

commented Mar 7, 2019

/lgtm

Thanks @MorrisLaw!

/assign @msau42 @jsafrane
for approval

@jsafrane

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

I still don't like interface named Attacher, but I did not find a better name. So...

/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsafrane, MorrisLaw

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 merged commit 1573881 into kubernetes-csi:master Mar 7, 2019

2 of 3 checks passed

tide Not mergeable. Needs approved, lgtm labels.
Details
cla/linuxfoundation MorrisLaw authorized
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Consistent sidecars automation moved this from In progress to Done Mar 7, 2019

@MorrisLaw MorrisLaw deleted the MorrisLaw:consistent-sidecars branch Mar 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.