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

Clean up fake mounters. #64426

Merged
merged 1 commit into from Jun 20, 2018
Merged

Clean up fake mounters. #64426

merged 1 commit into from Jun 20, 2018

Conversation

cofyc
Copy link
Member

@cofyc cofyc commented May 29, 2018 •
edited

What this PR does / why we need it:

Fixes #61502

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

list of fake mounters:

  • (keep) pkg/util/mount.FakeMounter
  • (removed) pkg/kubelet/cm.fakeMountInterface:
  • (inherit from mount.FakeMounter) pkg/util/mount.fakeMounter
  • (inherit from mount.FakeMounter) pkg/util/removeall.fakeMounter
  • (removed) pkg/volume/host_path.fakeFileTypeChecker

Release note:

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 29, 2018
@cofyc cofyc force-pushed the remove_unnecessary_fakemounters branch 2 times, most recently from 46bcacb to 43907b2 Compare May 29, 2018 07:43
@feiskyer
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 29, 2018
@cofyc
Copy link
Member Author

cofyc commented May 29, 2018

@feiskyer Thanks!

/assign @msau42 @jsafrane

@jsafrane
Copy link
Member

@cofyc, please apply for membership using this process, it will simplify things for you. I can sponsor you, perhaps @msau42 can too. https://github.com/kubernetes/community/blob/master/community-membership.md#member

@jsafrane
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 29, 2018
@cofyc
Copy link
Member Author

cofyc commented May 29, 2018 •
edited

@jsafrane Thanks! I had applied for membership a few days ago. I'm waiting for response.

@msau42
Copy link
Member

msau42 commented May 29, 2018

/lgtm
Thanks!

@cofyc
Copy link
Member Author

cofyc commented May 31, 2018

/assign @brendandburns @dchen1107 @lavalamp @smarterclayton @thockin
hi, Could you help approve this?

lavalamp
lavalamp reviewed May 31, 2018
View reviewed changes
pkg/util/mount/exec_mount_test.go Outdated
mounter := NewExecMounter(exec, wrappedMounter)

mounter.Unmount(destinationPath)
}

/* Fake wrapped mounter */
type fakeMounter struct {
t *testing.T
*FakeMounter
*testing.T
Copy link
Member

Choose a reason for hiding this comment

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

keep the name lowercase?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. T changed to t. (fakeMounter needs to embed FakeMounter to inherit methods so it cannot be lowered)

thockin
thockin reviewed May 31, 2018
View reviewed changes
pkg/util/mount/fake.go
@@ -29,6 +29,7 @@ import (
type FakeMounter struct {
MountPoints []MountPoint
Log []FakeAction
Filesystem map[string]FileType
Copy link
Member

Choose a reason for hiding this comment

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

This type should be moved to a testing/ subdir so it doesn't get linked into non-test code. Won't block this PR on it.

Copy link
Member Author

@cofyc cofyc Jun 1, 2018 •
edited

Choose a reason for hiding this comment

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

OK. It'll change a lot of go imports and BUILD files, I'll create another PR for this.

@thockin
Copy link
Member

thockin commented May 31, 2018

/approve

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 31, 2018
@cofyc cofyc force-pushed the remove_unnecessary_fakemounters branch from 43907b2 to 390898a Compare June 1, 2018 02:05
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 1, 2018
@cofyc
Copy link
Member Author

cofyc commented Jun 1, 2018

@jsafrane @msau42

I've made a minor change, please help re-lgtm it, thanks!

  • fakeMounter.T changed to fakeMounter.t in pkg/util/mount/exec_mount_test.go

@jsafrane
Copy link
Member

jsafrane commented Jun 1, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 1, 2018
@cofyc cofyc force-pushed the remove_unnecessary_fakemounters branch from 390898a to 9bc905f Compare June 2, 2018 07:41
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 2, 2018
@cofyc cofyc force-pushed the remove_unnecessary_fakemounters branch from 9bc905f to 40c3937 Compare June 2, 2018 07:55
@cofyc
Copy link
Member Author

cofyc commented Jun 2, 2018 •
edited

@jsafrane
Resolved conflicts with master (#63143), please take a look at again.

feiskyer
feiskyer reviewed Jun 3, 2018
View reviewed changes
Copy link
Member

@feiskyer feiskyer 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 Jun 3, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cofyc, feiskyer, jsafrane, msau42, thockin

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

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

4 similar comments
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 64142, 64426, 62910, 63942, 64548). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 148350d into kubernetes:master Jun 20, 2018
@cofyc cofyc deleted the remove_unnecessary_fakemounters branch June 22, 2018 11:06
@wongma7 wongma7 mentioned this pull request Jul 3, 2018
Merged
@cofyc cofyc mentioned this pull request Jul 6, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lavalamp lavalamp lavalamp left review comments

@feiskyer feiskyer feiskyer left review comments

@thockin thockin thockin left review comments

@brendandburns brendandburns Awaiting requested review from brendandburns

Assignees

@lavalamp lavalamp

@feiskyer feiskyer

@smarterclayton smarterclayton

@jsafrane jsafrane

@thockin thockin

@brendandburns brendandburns

@dchen1107 dchen1107

@msau42 msau42

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. release-note-none Denotes a PR that doesn't merit a release note. 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.

Clean up fake mounters