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.

Sign up for GitHub

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

Jump to bottom

Add common functionality required for all in-tree to CSI Migration shim code pieces #73043

Merged
merged 2 commits into from
Jan 18, 2019
Merged

Add common functionality required for all in-tree to CSI Migration shim code pieces #73043

merged 2 commits into from
Jan 18, 2019

Conversation

davidz627
Copy link
Contributor

Many of the CSI Migration shim pieces depend on some common functionality, mostly revolving around feature flags and this plugin interface method. Would be nice to get these pieces in first so that the work on different pieces of migration can continue in parallel without rebasing issues.

/kind feature
/sig storage
/assign @saad-ali @jsafrane
/cc @ddebroy @leakingtapan @dims

NONE

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. sig/storage Categorizes an issue or PR as relevant to SIG Storage. release-note-none Denotes a PR that doesn't merit a release note. labels Jan 17, 2019
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. sig/apps Categorizes an issue or PR as relevant to SIG Apps. labels Jan 17, 2019
@davidz627
Copy link
Contributor Author

/cc @msau42 @kubernetes/sig-storage-pr-reviews
xref: kubernetes/enhancements#625

@k8s-ci-robot k8s-ci-robot requested a review from msau42 January 17, 2019 20:00
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 17, 2019
saad-ali
saad-ali approved these changes Jan 17, 2019
View reviewed changes
Copy link
Member

@saad-ali saad-ali left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

pkg/volume/util/operationexecutor/operation_generator.go Outdated
checkNodeCapabilitiesBeforeMount: checkNodeCapabilitiesBeforeMount,
blkUtil: blkUtil,
blkUtil: blkUtil,
Copy link
Member

Choose a reason for hiding this comment

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

nit: why was the spacing here changed? Doesn't gofmt enforce this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think my gofmt must be wonky, I've been getting a lot of weird diffs like this that I have to fix manually

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 17, 2019
@davidz627 davidz627 force-pushed the feature/migrationCommon branch from 9927d60 to 379783a Compare January 17, 2019 21:13
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 17, 2019
@davidz627
Copy link
Contributor Author

@saad-ali fixed the gofmt issue, could you re-lgtm

@saad-ali
Copy link
Member

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 17, 2019
@saad-ali
Copy link
Member

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 17, 2019
@davidz627 davidz627 force-pushed the feature/migrationCommon branch from 379783a to 3dc0387 Compare January 17, 2019 21:42
@k8s-ci-robot k8s-ci-robot added area/apiserver area/kubelet sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 17, 2019
@k8s-ci-robot k8s-ci-robot added sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/release Categorizes an issue or PR as relevant to SIG Release. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Jan 17, 2019
@davidz627 davidz627 force-pushed the feature/migrationCommon branch 2 times, most recently from 5f12457 to a7e8442 Compare January 17, 2019 21:54
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 17, 2019
@saad-ali
Copy link
Member

/hold cancel
/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jan 17, 2019
@davidz627
Copy link
Contributor Author

/retest

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 17, 2019
@davidz627 davidz627 force-pushed the feature/migrationCommon branch from f4acf34 to 384fce6 Compare January 17, 2019 22:44
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Jan 17, 2019 •
edited
Loading

@davidz627: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-local-e2e-containerized 5f1245749717e102edfc9278212997357753a44b link /test pull-kubernetes-local-e2e-containerized

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

saad-ali
saad-ali approved these changes Jan 17, 2019
View reviewed changes
Copy link
Member

@saad-ali saad-ali left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 17, 2019
@davidz627
Copy link
Contributor Author

/retest

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidz627, saad-ali

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 7f0e04a into kubernetes:master Jan 18, 2019
@davidz627 davidz627 deleted the feature/migrationCommon branch January 18, 2019 19:12
@adisky adisky mentioned this pull request Feb 8, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@saad-ali saad-ali saad-ali approved these changes

@ddebroy ddebroy Awaiting requested review from ddebroy

@dims dims Awaiting requested review from dims

@leakingtapan leakingtapan Awaiting requested review from leakingtapan

@msau42 msau42 Awaiting requested review from msau42

Assignees

@jsafrane jsafrane

@saad-ali saad-ali

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver area/kubelet cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note-none Denotes a PR that doesn't merit a release note. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/release Categorizes an issue or PR as relevant to SIG Release. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. sig/storage Categorizes an issue or PR as relevant to SIG Storage. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@davidz627 @saad-ali @k8s-ci-robot @jsafrane