Warn when a user tries to set a profile name that is unlikely to be valid #4999
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Welcome @bhanu011! |
k8s-ci-robot
added
do-not-merge/work-in-progress
|
Hi @bhanu011. 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 Once the patch is verified, the new status will be reflected by the 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
added
the
needs-ok-to-test
|
Can one of the admins verify this patch? |
k8s-ci-robot
added
the
size/M
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: bhanu011 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 |
tstromberg
suggested changes
Aug 6, 2019
There was a problem hiding this comment.
First, this looks good. I'd love to see a test for a passing TestValidateProfile.
I hope you don't mind, but I've added a bunch of tips for writing idiomatic Go & Kubernetes code. I know that this PR is a WIP, but I hope you are able to find the comments useful. Thank you for proposing this fix!
|
@minikube-bot OK to test |
Thanks a lot for review comments mentioning why it has to be in that way. I am learning a lot from this contribution |
bhanu-lab
commented
Aug 8, 2019
bhanu-lab
changed the title
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
bhanu-lab
changed the title
bhanu-lab
changed the title
|
Fixes #4883 |
bhanu-lab
changed the title
|
@minikube-bot OK to test |
|
Integration tests appear to be unrelated flakes: HyperKit_macOS: |
tstromberg
changed the title
medyagh
requested changes
Aug 21, 2019
| // not validating when it is default profile | ||
| errProfile, ok := ValidateProfile(profile) | ||
| if !ok && errProfile != nil { | ||
| out.FailureT(errProfile.Msg) |
There was a problem hiding this comment.
this PR looks good ! but we need to add a message to the end user, how to create a profile !
this PR will make it minikube start to be the only way to create a profile.
|
@bhanu011 thank for taking this PR ! I just compiled your code and I tried to do minikube ssh on a not existing profile (px): it seems like the your ValidarteProfile func was not called ! |
|
one good place to do test this would be integration tests. I suggest adding an integration tests to https://github.com/kubernetes/minikube/blob/master/test/integration/fn_profile.go in which you can try to run "minikube logs" on a profile that doesn't exist and expect the output of the message to be yours. and also please make sure to error to the user how to create a profile. for example here is more info on integration testing : one tip on running intregation test: you can run only One Test by this command: |
|
@bhanu011 - Would you like any help to move this PR along? |
|
@bhanu011 are you still interested in finishing up this PR? |
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
|
@minikube-bot OK to test |
Travis tests have failedHey @bhanu011, 1st Buildmake test2nd Buildmake testTravisBuddy Request Identifier: 64c54410-eac4-11e9-90a0-7556777797a6 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Need an early review before merging