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

addons: Fixes multiple files behavior in files rootfs #3501

Merged
merged 2 commits into from
Feb 15, 2019

Conversation

laozc
Copy link
Contributor

@laozc laozc commented Jan 2, 2019

Some files may be left uncopied to the VM when there are multiple
files in the .minikube/files directory as the code misinterprets
the last parent dir as parent dirs for all files, which results
some files with the same name were not copied.

This patch fixes the behavior.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 2, 2019
@minikube-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@tstromberg
Copy link
Contributor

Nice work. Any chance you could add a unit or integration test to ensure that we don't accidentally reverse this behavior in the future?

@tstromberg tstromberg changed the title Fixes multiple files behavior in files rootfs addons: Fixes multiple files behavior in files rootfs Jan 16, 2019
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 16, 2019
@laozc
Copy link
Contributor Author

laozc commented Jan 21, 2019

Is the patch to add unit test in a large size than expected?

@tstromberg
Copy link
Contributor

Is the patch to add unit test in a large size than expected?

Yeah. I'm going to have to give some thought to the use of afero here. Give me a few days. Thanks!

@laozc
Copy link
Contributor Author

laozc commented Jan 25, 2019

Sure.
Let me know if there is a better way to handle it.

@@ -247,10 +248,10 @@ var Addons = map[string]*Addon{
}

func AddMinikubeDirAssets(assets *[]CopyableFile) error {
if err := addMinikubeDirToAssets(constants.MakeMiniPath("addons"), constants.AddonsPath, assets); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

I needed some time to think about this PR, and while I like the concept of afero.Fs, I don't think this current approach of embedding it around the minikube code base works for me. I feel that passing nil to a function is generally a sign of a hastily tacked on API. Then again, so is having a MakeMiniPath function in a constants package (not your fault!)

Feel free to correct me - but I don't feel that using the real filesystem here will negatively impact our overall test run time.

It's less elegant, but what do you think about adding a helper to test_addons.go, which sets up a new filesystem directory for minikube:

func setupTestDir() (path, error) {
  path, err := ioutil.TempDir()
  os.Setenv(constants.MinikubeHome, path)
  return path, err
}

Then you can gather a list of files written to disk via filepath.Walk, and compare it against expectations:

path, err := setupTestDir()
got := filepath.Walk(path)

    if diff := cmp.Diff(got, want); diff != "" {
        t.Errorf("files differ: (-want +got)\n%s", diff)
    }

It's certainly less elegant, but what are your thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the advice.
Changed the test cases to use a temporary dir for Minikube config as suggested.
It should eliminate the code change in non-relevant files by now.

Some files may be left uncopied to the VM when there are multiple
files in the .minikube/files directory as the code misinterprets
the last parent dir as parent dirs for all files, which results
some files with the same name were not copied.

This patch fixes the behavior.
@laozc laozc force-pushed the master branch 3 times, most recently from 979d177 to 6d89958 Compare February 10, 2019 12:59
afero introduced for mocking a filesystem.
@tstromberg
Copy link
Contributor

/lgtm

@minikube-bot OK to test

@k8s-ci-robot k8s-ci-robot added 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 Feb 13, 2019
@tstromberg
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: laozc, tstromberg

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

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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants