Don't allow creating profile by profile command #6672

aallbrig · 2020-02-18T00:14:17Z

The Change Set

This updates profile to no longer create profiles that do not exist and provides feedback to user on how to create the non-existent profile.

Just One Snag...

One thing brought up in the issue that doesn't appear to be true is...

however, 'minikube profile pname'
should still change the profile to 'pname' if it exists. but should not create a new one.

However; minikube profile pname does not change profile to pname before this change set. This seems like it would be new functionality

$ minikube create -p profile
$ minikube profile pname

Instead of renaming profile to pname, it instead creates a pname profile that needs to be deleted.

TLDR; This aspect of issue #6647 may be a new feature request because this behavior does not exist today.

See below screenshots.

k8s-ci-robot · 2020-02-18T00:14:25Z

Hi @aallbrig. 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 · 2020-02-18T00:15:20Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: aallbrig
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:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

codecov-io · 2020-02-18T00:20:07Z

Codecov Report

Merging #6672 into master will increase coverage by 0.01%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #6672      +/-   ##
==========================================
+ Coverage   38.56%   38.58%   +0.01%     
==========================================
  Files         142      142              
  Lines        8642     8639       -3     
==========================================
  Hits         3333     3333              
+ Misses       4893     4890       -3     
  Partials      416      416

Impacted Files	Coverage Δ
cmd/minikube/cmd/config/profile.go	`0% <0%> (ø)`	⬆️

minikube-bot · 2020-02-18T00:21:03Z

Can one of the admins verify this patch?

medyagh · 2020-02-18T01:26:08Z

Thank you for the pr. Do you mind adding integration tests for this ?

You could look up examples of functional tests and add one more tests under functional tests

aallbrig · 2020-02-18T02:16:14Z

@medyagh I can but not tonight. I’m spending time with the fiancé for the rest of the night.

It’s 9:14 p.m. EST. I’ll see about getting this done between 7 a.m. and 8 a.m. tomorrow.

aallbrig · 2020-02-18T10:59:23Z

@medyagh I'm on it now

aallbrig · 2020-02-18T12:15:52Z

@medyagh I found that there were integration tests for profile already existing! I created tests for nonexistent profiles and the desired behavior described in the issue.

I look forward to your review. Its 7:12 a.m. EST now. I'll have a block of time during lunch and another block of time after work to address feedback you (or others) provide.

aallbrig · 2020-02-19T00:04:54Z

@medyagh Is there anything else I can do?

RA489 · 2020-02-19T05:23:39Z

/ok-to-test

minikube-pr-bot · 2020-02-19T05:30:47Z

Error: running mkcmp: exit status 1

medyagh · 2020-02-19T17:27:34Z

@aallbrig thank you so much for this PR ! this looks good to me just a small nit. and thank you for adding integraiotn tests for it too

medyagh

this PR is short and sweet and is great ! and fixes an annoyance for the users.
thank you just a small request to improve the integration tests (parse the json output instead of parsing the stdoutput )

medyagh · 2020-02-19T17:41:42Z

test/integration/functional_test.go

+	if err != nil {
+		t.Errorf("%s failed: %v", rr.Args, err)
+	}
+	for _, line := range []string{fmt.Sprintf("Created a new profile : %s", nonexistentProfile), fmt.Sprintf("minikube profile was successfully set to %s", nonexistentProfile)} {


how about instead of parsing the output, do a
"minikube profile list --output=json" and expect not to see the "lis" in the created profiles?

Absolutely! I’ll update it today

aallbrig · 2020-02-19T23:27:30Z

@medyagh Touching base: doing a trivia night with friends & S.Os. My plan is to do the refactor later today. Cheers!

medyagh · 2020-02-20T00:27:32Z

trivia night with friends & S.Os. My plan is to do the refactor later today. Cheers!

thanks for keeping me up to date ! have fun at the trivia :)

aallbrig · 2020-02-20T03:58:22Z

@medyagh Updated tests to use JSON output to determine expected behavior. Please let me know how I can improve this change set :)

minikube-pr-bot · 2020-02-20T04:10:30Z

All Times minikube: [ 96.652477 93.172916 92.120139]
All Times Minikube (PR 6672): [ 90.669450 94.047599 94.918137]

Average minikube: 93.981844
Average Minikube (PR 6672): 93.211729

Averages Time Per Log

+----------------------+-----------+--------------------+
|         LOG          | MINIKUBE  | MINIKUBE (PR 6672) |
+----------------------+-----------+--------------------+
| minikube v           |  0.204334 |           0.198410 |
| Creating kvm2        | 21.452039 |          19.962150 |
| Preparing Kubernetes | 49.761131 |          50.326659 |
| Pulling images       |           |                    |
| Launching Kubernetes | 20.886641 |          20.705711 |
| Waiting for cluster  |  0.054255 |           0.059329 |
+----------------------+-----------+--------------------+

aallbrig · 2020-02-20T10:59:46Z

Woohoo!

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 18, 2020

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 18, 2020

k8s-ci-robot requested review from josedonizetti and medyagh February 18, 2020 00:14

aallbrig changed the title ~~Do not create profiles that do not exist~~ Profile command should not create non existent profiles Feb 18, 2020

Do not create profiles that do not exist

089deef

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 Feb 18, 2020

aallbrig force-pushed the profile-command-improvement branch from 07a847d to 089deef Compare February 18, 2020 11:07

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 18, 2020

Add tests around nonexistent profiles

ff873d2

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 18, 2020

aallbrig added 2 commits February 18, 2020 19:00

word -> line

addb2e6

Merge branch 'master' into profile-command-improvement

f9f828b

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 19, 2020

medyagh approved these changes Feb 19, 2020

View reviewed changes

medyagh requested changes Feb 19, 2020

View reviewed changes

Use JSON output to determine desired behavior

020c0f3

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 20, 2020

medyagh approved these changes Feb 20, 2020

View reviewed changes

medyagh merged commit a5c76f8 into kubernetes:master Feb 20, 2020

medyagh changed the title ~~Profile command should not create non existent profiles~~ don't allow creating profile by profile command Feb 20, 2020

medyagh changed the title ~~don't allow creating profile by profile command~~ Don't allow creating profile by profile command Feb 20, 2020

fmotrifork mentioned this pull request Feb 24, 2020

minikube 1.7.3 fishworks/fish-food#585

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't allow creating profile by profile command #6672

Don't allow creating profile by profile command #6672

aallbrig commented Feb 18, 2020

k8s-ci-robot commented Feb 18, 2020

k8s-ci-robot commented Feb 18, 2020

codecov-io commented Feb 18, 2020

minikube-bot commented Feb 18, 2020

medyagh commented Feb 18, 2020

aallbrig commented Feb 18, 2020

aallbrig commented Feb 18, 2020

aallbrig commented Feb 18, 2020

aallbrig commented Feb 19, 2020

RA489 commented Feb 19, 2020

minikube-pr-bot commented Feb 19, 2020

medyagh commented Feb 19, 2020 •

edited

Loading

medyagh left a comment

medyagh Feb 19, 2020

aallbrig Feb 19, 2020 •

edited

Loading

aallbrig commented Feb 19, 2020

medyagh commented Feb 20, 2020

aallbrig commented Feb 20, 2020

minikube-pr-bot commented Feb 20, 2020

aallbrig commented Feb 20, 2020

Don't allow creating profile by profile command #6672

Don't allow creating profile by profile command #6672

Conversation

aallbrig commented Feb 18, 2020

The Change Set

Just One Snag...

k8s-ci-robot commented Feb 18, 2020

k8s-ci-robot commented Feb 18, 2020

codecov-io commented Feb 18, 2020

Codecov Report

minikube-bot commented Feb 18, 2020

medyagh commented Feb 18, 2020

aallbrig commented Feb 18, 2020

aallbrig commented Feb 18, 2020

aallbrig commented Feb 18, 2020

aallbrig commented Feb 19, 2020

RA489 commented Feb 19, 2020

minikube-pr-bot commented Feb 19, 2020

medyagh commented Feb 19, 2020 • edited Loading

medyagh left a comment

Choose a reason for hiding this comment

medyagh Feb 19, 2020

Choose a reason for hiding this comment

aallbrig Feb 19, 2020 • edited Loading

Choose a reason for hiding this comment

aallbrig commented Feb 19, 2020

medyagh commented Feb 20, 2020

aallbrig commented Feb 20, 2020

minikube-pr-bot commented Feb 20, 2020

aallbrig commented Feb 20, 2020

medyagh commented Feb 19, 2020 •

edited

Loading

aallbrig Feb 19, 2020 •

edited

Loading