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

[WIP] Make dynamic persistent volumes path readable and configurable #5400

Closed
wants to merge 4 commits into from

Conversation

11janci
Copy link
Contributor

@11janci 11janci commented Sep 18, 2019

For #5144 & #3318

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 18, 2019
@k8s-ci-robot
Copy link
Contributor

Hi @11janci. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 18, 2019
@minikube-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 18, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: 11janci
To complete the pull request process, please assign sharifelgamal
You can assign the PR to them by writing /assign @sharifelgamal in a comment when ready.

The full list of commands accepted by this bot can be found 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

@TravisBuddy
Copy link

Travis tests have failed

Hey @11janci,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

1st Build

View build log

make test
which go-bindata || GO111MODULE=off GOBIN=/home/travis/gopath/bin go get github.com/jteeuwen/go-bindata/...
PATH="/home/travis/gopath/bin:/home/travis/.gimme/versions/go1.12.9.linux.amd64/bin:/home/travis/bin:/home/travis/bin:/home/travis/.local/bin:/usr/local/lib/jvm/openjdk11/bin:/opt/pyenv/shims:/home/travis/.phpenv/shims:/home/travis/perl5/perlbrew/bin:/home/travis/.nvm/versions/node/v8.12.0/bin:/home/travis/.rvm/gems/ruby-2.5.3/bin:/home/travis/.rvm/gems/ruby-2.5.3@global/bin:/home/travis/.rvm/rubies/ruby-2.5.3/bin:/home/travis/gopath/bin:/home/travis/.gimme/versions/go1.11.1.linux.amd64/bin:/usr/local/maven-3.6.0/bin:/usr/local/cmake-3.12.4/bin:/usr/local/clang-7.0.0/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/snap/bin:/home/travis/.rvm/bin:/home/travis/.phpenv/bin:/opt/pyenv/bin:/home/travis/.yarn/bin:/home/travis/gopath/bin" go-bindata -nomemcopy -o pkg/minikube/assets/assets.go -pkg assets deploy/addons/...
gofmt -s -w pkg/minikube/assets/assets.go
which go-bindata || GO111MODULE=off GOBIN=/home/travis/gopath/bin go get github.com/jteeuwen/go-bindata/...
/home/travis/gopath/bin/go-bindata
PATH="/home/travis/gopath/bin:/home/travis/.gimme/versions/go1.12.9.linux.amd64/bin:/home/travis/bin:/home/travis/bin:/home/travis/.local/bin:/usr/local/lib/jvm/openjdk11/bin:/opt/pyenv/shims:/home/travis/.phpenv/shims:/home/travis/perl5/perlbrew/bin:/home/travis/.nvm/versions/node/v8.12.0/bin:/home/travis/.rvm/gems/ruby-2.5.3/bin:/home/travis/.rvm/gems/ruby-2.5.3@global/bin:/home/travis/.rvm/rubies/ruby-2.5.3/bin:/home/travis/gopath/bin:/home/travis/.gimme/versions/go1.11.1.linux.amd64/bin:/usr/local/maven-3.6.0/bin:/usr/local/cmake-3.12.4/bin:/usr/local/clang-7.0.0/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/snap/bin:/home/travis/.rvm/bin:/home/travis/.phpenv/bin:/opt/pyenv/bin:/home/travis/.yarn/bin:/home/travis/gopath/bin" go-bindata -nomemcopy -o pkg/minikube/translate/translations.go -pkg translate translations/...
gofmt -s -w pkg/minikube/translate/translations.go
./test.sh
= go mod ================================================================
ok
= make lint =============================================================
golangci/golangci-lint info checking GitHub for tag 'v1.18.0'
golangci/golangci-lint info found version: 1.18.0 for v1.18.0/linux/amd64
golangci/golangci-lint info installed out/linters/golangci-lint
cmd/minikube/cmd/start.go:299:9: elseif: can replace 'else {if cond {}}' with 'else if cond {}' (gocritic)
	} else {
	       ^
Makefile:335: recipe for target 'lint-ci' failed
make[1]: *** [lint-ci] Error 1
= boilerplate ===========================================================
ok
= schema_check ==========================================================
ok
= go test ===============================================================
ok  	k8s.io/minikube/cmd/minikube/cmd	0.081s	coverage: 16.0% of statements
ok  	k8s.io/minikube/cmd/minikube/cmd/config	0.088s	coverage: 18.7% of statements
ok  	k8s.io/minikube/pkg/drivers	0.020s	coverage: 13.4% of statements
ok  	k8s.io/minikube/pkg/drivers/kvm	0.062s	coverage: 2.3% of statements
ok  	k8s.io/minikube/pkg/minikube/assets	0.035s	coverage: 61.8% of statements
ok  	k8s.io/minikube/pkg/minikube/bootstrapper	3.423s	coverage: 73.3% of statements
ok  	k8s.io/minikube/pkg/minikube/bootstrapper/kubeadm	0.055s	coverage: 30.6% of statements
ok  	k8s.io/minikube/pkg/minikube/cluster	0.655s	coverage: 53.0% of statements
ok  	k8s.io/minikube/pkg/minikube/config	0.019s	coverage: 76.0% of statements
ok  	k8s.io/minikube/pkg/minikube/cruntime	0.024s	coverage: 62.4% of statements
ok  	k8s.io/minikube/pkg/minikube/extract	0.009s	coverage: 56.7% of statements
ok  	k8s.io/minikube/pkg/minikube/kubeconfig	0.049s	coverage: 75.9% of statements
ok  	k8s.io/minikube/pkg/minikube/logs	0.036s	coverage: 1.4% of statements
ok  	k8s.io/minikube/pkg/minikube/machine	0.036s	coverage: 11.3% of statements
ok  	k8s.io/minikube/pkg/minikube/notify	0.023s	coverage: 78.6% of statements
ok  	k8s.io/minikube/pkg/minikube/out	0.017s	coverage: 70.3% of statements
ok  	k8s.io/minikube/pkg/minikube/problem	0.007s	coverage: 100.0% of statements
ok  	k8s.io/minikube/pkg/minikube/proxy	0.015s	coverage: 100.0% of statements
ok  	k8s.io/minikube/pkg/minikube/registry	0.014s	coverage: 100.0% of statements
ok  	k8s.io/minikube/pkg/minikube/service	0.034s	coverage: 35.9% of statements
ok  	k8s.io/minikube/pkg/minikube/sshutil	0.118s	coverage: 75.0% of statements
ok  	k8s.io/minikube/pkg/minikube/translate	0.004s	coverage: 8.4% of statements
ok  	k8s.io/minikube/pkg/minikube/tunnel	1.989s	coverage: 64.5% of statements
ok  	k8s.io/minikube/pkg/util	1.019s	coverage: 62.0% of statements
ok  	k8s.io/minikube/pkg/util/lock	0.008s	coverage: 59.1% of statements
ok  	k8s.io/minikube/pkg/util/retry	0.006s	coverage: 0.0% of statements
ok
Makefile:232: recipe for target 'test' failed
make: *** [test] Error 4
TravisBuddy Request Identifier: f4afba20-da65-11e9-b256-8f6de709717d

@codecov-io
Copy link

codecov-io commented Oct 4, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@9db86d9). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #5400   +/-   ##
=========================================
  Coverage          ?   37.69%           
=========================================
  Files             ?      103           
  Lines             ?     7616           
  Branches          ?        0           
=========================================
  Hits              ?     2871           
  Misses            ?     4368           
  Partials          ?      377

@tstromberg
Copy link
Contributor

@minikube-bot OK to test

@11janci
Copy link
Contributor Author

11janci commented Oct 10, 2019

/retest

@k8s-ci-robot
Copy link
Contributor

@11janci: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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.

@11janci 11janci force-pushed the jjanik-pvc-path branch 2 times, most recently from 9bcff48 to 9218f12 Compare October 15, 2019 21:23
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 15, 2019
@11janci
Copy link
Contributor Author

11janci commented Oct 16, 2019

/retest

@k8s-ci-robot
Copy link
Contributor

@11janci: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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.

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 16, 2019
@tstromberg
Copy link
Contributor

@minikube-bot OK to test

@tstromberg
Copy link
Contributor

Change looks good, but it isn't clear to me what issue this is fixing. Can you add a description as to why this change is happening?

@11janci
Copy link
Contributor Author

11janci commented Oct 24, 2019

@tstromberg It is for #5144 and #3318 (updated the PR desc). However I'm hitting a wall testing it. Can't figure out why the tests here are failing, am trying to revert changes to see where it breaks. Also, I'm not able to test it locally without the minikube ISO and can't build the ISO (is failing on my mac) 😒 I should be able to get it from https://storage.googleapis.com/minikube-builds/5400/minikube-testing.iso, but for that I need the tests here to pass 🤕

@medyagh
Copy link
Member

medyagh commented Nov 6, 2019

@11janci do you mind providing examples ? maybe in the PR descrption and also add related docs in the site?

@11janci
Copy link
Contributor Author

11janci commented Nov 6, 2019

@medyagh you mean examples of usage? This is a work in progress and I'd like to get it working first, however don't have the capacity to work on it right now :/

@nanikjava
Copy link
Contributor

.......... Also, I'm not able to test it locally without the minikube ISO and can't build the ISO (is failing on my mac) I should be able to get it from https://storage.googleapis.com/minikube-builds/5400/minikube-testing.iso, but for that I need the tests here to pass

@11janci If you need help in running the testing on local machine happy to help

@tstromberg
Copy link
Contributor

tstromberg commented Dec 9, 2019

Need any help with this?

The integration test looks good. I just don't understand exactly what the end goal is, and how far along this PR is at achieving it.

@11janci
Copy link
Contributor Author

11janci commented Dec 10, 2019

@tstromberg, @ALL yeah as mentioned above, I currently don't have the capacity to work on this (for at least another 2 months). If anybody wants to take over, feel free to pull my changes and finish it off.

The end goal is to make the dynamic PV path
a) readable - using the PVC's name instead of the UUID (#3318)
b) configurable - sending a value to the provisioner to overwrite the default path (#5144)

I was adding and removing my changes as I was trying to test it so check out my previous commits, especially the first one.

@tstromberg
Copy link
Contributor

tstromberg commented Feb 5, 2020

Thank you for the update! I'm closing this PR until someone is ready to take a look at it again. Please feel free to re-open it once you or someone else has time.

Related: #6156

@tstromberg tstromberg closed this Feb 5, 2020
@11janci 11janci deleted the jjanik-pvc-path branch July 14, 2020 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants