-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Add ability to get json for "minikube service list" #15831
Conversation
|
Welcome @OmSaran! |
Hi @OmSaran. 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. |
Can one of the admins verify this patch? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also add a simple test to
minikube/test/integration/functional_test.go
Lines 1396 to 1397 in 89dbf51
// validateServiceCmd asserts basic "service" command functionality | |
func validateServiceCmd(ctx context.Context, t *testing.T, profile string) { |
Similar to
minikube/test/integration/functional_test.go
Lines 1337 to 1338 in 89dbf51
// docs: Run `minikube profile list -o JSON` and make sure the profiles are correctly listed as JSON output | |
t.Run("profile_json_output", func(t *testing.T) { |
cmd/minikube/cmd/service_list.go
Outdated
switch output { | ||
case "table": | ||
printServicesTable(serviceURLs, co) | ||
case "json": | ||
printServicesJSON(serviceURLs) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should be a default case that outputs something like:
exit.Message(reason.Usage, fmt.Sprintf("invalid output format: %s. Valid values: 'table', 'json'", output))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching this, fixed it!
@OmSaran do you have any questions on how to add integration test? |
Done! Please take a look |
I took a stab at it, let me know if this looks okay! |
/ok-to-test |
@OmSaran please check the failed test https://github.com/kubernetes/minikube/actions/runs/4188409014/jobs/7262233741 to be clear we dont really need to do a full Service test again there (such as deploying a new service) we could just ensure the service list -o=json is a valid json and does have the service we expect there in json |
@OmSaran you can run the functional tests on your own machine using "make functional" |
This time I tested it using a linux machine and it was successful. Thanks!
Previously it was not working on my local MacOS machine as it was getting hung in |
kvm2 driver with docker runtime
Times for minikube start: 54.1s 50.5s 55.2s 51.7s 57.3s Times for minikube ingress: 24.7s 25.2s 25.3s 23.8s 24.7s docker driver with docker runtime
Times for minikube start: 26.3s 27.8s 27.6s 26.5s 27.9s Times for minikube ingress: 22.1s 22.1s 20.1s 21.6s 22.1s docker driver with containerd runtime
Times for minikube ingress: 28.6s 19.6s 32.6s 20.6s 32.6s Times for minikube start: 22.8s 22.3s 22.0s 21.6s 21.8s |
These are the flake rates of all failed tests.
To see the flake rates of all tests by environment, click here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also run make generate-docs
to update the website documentation with the new flag, thanks
I did, but I see some other changes too which I did not intend but was a consequence of |
kvm2 driver with docker runtime |
These are the flake rates of all failed tests.
To see the flake rates of all tests by environment, click here. |
That's just a change from a previous PR that didn't get the command run on it, it's fine to include it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My only concern is that if using Docker on macOS we strip out the service URLs for table output but not for JSON which will cause a discrepancy between the two outputs.
To solve this we can loop through the services before the switch statement to remove the URLs so the outputs match.
I fixed this in the new commit, please take a look. |
kvm2 driver with docker runtime
Times for minikube start: 55.6s 56.1s 54.8s 55.3s 55.0s Times for minikube ingress: 26.2s 24.3s 28.7s 29.3s 27.7s docker driver with docker runtime
Times for minikube start: 28.4s 27.0s 26.9s 27.8s 27.4s Times for minikube ingress: 22.1s 23.1s 21.1s 22.2s 23.1s docker driver with containerd runtime
Times for minikube start: 21.5s 22.8s 22.9s 21.7s 22.3s Times for minikube ingress: 32.6s 21.6s 19.6s 19.6s 31.6s |
These are the flake rates of all failed tests.
To see the flake rates of all tests by environment, click here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, thanks for your PR!
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: OmSaran, spowelljr 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 |
fixes #15827
Before:
After: