service list cmd: display target port and name #6879

prasadkatti · 2020-03-04T06:30:35Z

Populate the target port in service list

Use port name for target port if provided. If not, use 'port'.
https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.17/#serviceport-v1-core

This is my first contribution to minikube. I am also fairly new to kubernetes. So please go easy on me. :)

Before

After

k8s-ci-robot · 2020-03-04T06:30:39Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: [email protected]

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. I understand the commands that are listed here.

k8s-ci-robot · 2020-03-04T06:30:44Z

Hi @prasadkatti. 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-03-04T06:31:16Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: prasadkatti
To complete the pull request process, please assign tstromberg
You can assign the PR to them by writing /assign @tstromberg 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

minikube-bot · 2020-03-04T06:39:02Z

Can one of the admins verify this patch?

prasadkatti · 2020-03-04T06:39:19Z

/assign @tstromberg

medyagh · 2020-03-04T08:00:02Z

@prasadkatti can you please put before and after output in the PR description ?

josedonizetti

@prasadkatti Thank you for this PR. I left a few comments.

josedonizetti · 2020-03-04T13:05:38Z

pkg/minikube/service/service.go

@@ -196,6 +197,13 @@ func printURLsForService(c typed_core.CoreV1Interface, ip, service, namespace st
 	urls := []string{}
 	portNames := []string{}
 	for _, port := range svc.Spec.Ports {
+
+		if port.Name != "" {
+			m[port.TargetPort.IntVal] = port.Name


I don't think the port name is useful by itself. I'd rather change to print something like name/port if name is available, if not only port.

I agree ! if needed feel free to change the profile column name, to reflect the name/port

Thanks for the review. I have pushed a change to address your feedback. I have added before/after images to issue description. The column name does need to be changed in my opinion. But I am not sure what would be the right name for the column.

josedonizetti · 2020-03-04T13:06:51Z

cmd/minikube/cmd/service_list.go

 				serviceURLs := strings.Join(serviceURL.URLs, "\n")

 				// if we are running Docker on OSX we empty the internal service URLs
 				if runtime.GOOS == "darwin" && cfg.Driver == oci.Docker {
 					serviceURLs = ""
 				}

-				data = append(data, []string{serviceURL.Namespace, serviceURL.Name, "", serviceURLs})
+				data = append(data, []string{serviceURL.Namespace, serviceURL.Name, servicePortNames, serviceURLs})


What we print for minikube service list must be consistent with what we print for minikube service <service-name>, so if we are printing name || port there, the same should be done here

Both minikube service list and minikube service <service-name> eventually call printURLsForService which is where I am making these changes. So the results should be consistent. Maybe the variable name needs to change?

medyagh · 2020-03-04T17:07:50Z

@prasadkatti welcome to the community of minikbue contributors :) happy to see your contribution !

prasadkatti · 2020-03-04T21:56:47Z

@medyagh - Thanks. I am hoping to learn more about minikube/kubernetes through this process. I have added the before/after images that you requested to the issue description. The column header does need to change in my opinion, but I am not sure what I should change it to. Please provide suggestions.

prasadkatti · 2020-03-04T22:13:37Z

Also, some tests are failing. I haven't quite figured that out yet. Are they failing because of these changes? I will need some help with that.

medyagh · 2020-03-04T23:09:51Z

@prasadkatti unit tests are failing please make sure to run make test locally to see the error or checkout the https://github.com/kubernetes/minikube/pull/6879/checks?check_run_id=486144522

seems like we need to update our unit test

|-----------|----------------|-------------|--------------------------------|
| NAMESPACE |      NAME      | TARGET PORT |              URL               |
|-----------|----------------|-------------|--------------------------------|
| default   | mock-dashboard | 0 0         | http://127.0.0.1:1111          |
|           |                |             | http://127.0.0.1:2222          |
|-----------|----------------|-------------|--------------------------------|
|-----------|----------------|-------------|--------------------------------|
| NAMESPACE |      NAME      | TARGET PORT |              URL               |
|-----------|----------------|-------------|--------------------------------|
| default   | mock-dashboard | 0 0         | http://127.0.0.1:1111          |
|           |                |             | http://127.0.0.1:2222          |
|-----------|----------------|-------------|--------------------------------|
FAIL
coverage: 85.8% of statements
FAIL	k8s.io/minikube/pkg/minikube/service	2.576s
ok  	k8s.io/minikube/pkg/minikube/sshutil	0.417s	coverage: 100.0% of statements
ok  	k8s.io/minikube/pkg/minikube/storageclass	0.020s	coverage: 97.2% of statements
ok  	k8s.io/minikube/pkg/minikube/translate	0.015s	coverage: 11.9% of statements
ok  	k8s.io/minikube/pkg/minikube/tunnel	1.749s	coverage: 64.2% of statements
ok  	k8s.io/minikube/pkg/util	0.682s	coverage: 70.6% of statements
ok  	k8s.io/minikube/pkg/util/lock	0.005s	coverage: 18.2% of statements
ok  	k8s.io/minikube/pkg/util/retry	0.002s	co

codecov-io · 2020-03-05T06:44:36Z

Codecov Report

Merging #6879 into master will decrease coverage by 0.5%.
The diff coverage is 40%.

@@            Coverage Diff            @@
##           master   #6879      +/-   ##
=========================================
- Coverage    38.3%   37.8%   -0.51%     
=========================================
  Files         142     142              
  Lines        8740    8875     +135     
=========================================
+ Hits         3348    3355       +7     
- Misses       4971    5093     +122     
- Partials      421     427       +6

Impacted Files	Coverage Δ
cmd/minikube/cmd/service_list.go	`9.09% <0%> (-1.26%)`	⬇️
pkg/minikube/service/service.go	`77.24% <50%> (-0.59%)`	⬇️
cmd/minikube/cmd/node_stop.go	`15% <0%> (-8.08%)`	⬇️
pkg/minikube/machine/status.go	`42.42% <0%> (-6.07%)`	⬇️
pkg/minikube/bootstrapper/bsutil/binaries.go	`5.4% <0%> (-5.13%)`	⬇️
pkg/minikube/driver/driver.go	`54.76% <0%> (-2.74%)`	⬇️
pkg/addons/addons.go	`43.52% <0%> (-1.6%)`	⬇️
cmd/minikube/cmd/config/open.go	`11.86% <0%> (-1.35%)`	⬇️
cmd/minikube/cmd/delete.go	`23.55% <0%> (-1.34%)`	⬇️
cmd/minikube/cmd/node_add.go	`22.72% <0%> (-1.28%)`	⬇️
... and 23 more

prasadkatti · 2020-03-05T07:12:44Z

@medyagh - I have fixed the unit tests. I had to update the tests like you suspected.

prasadkatti · 2020-03-08T21:35:23Z

Should I remove the wip: tag on this now?

medyagh · 2020-03-12T03:01:25Z

Thanks @prasadkatti I will review this tommorow

tstromberg · 2020-03-21T17:32:36Z

@medyagh - reminder to take a look at this PR when you get a chance

prasadkatti · 2020-03-27T21:02:40Z

Any update on this? Do I need to fix anything else here?

medyagh · 2020-03-28T02:23:18Z

/ok-to-test

medyagh · 2020-03-28T02:23:43Z

@prasadkatti thank you for your patience and sorry that took me so long to come back to this PR.
the changes look good to mne.
I will take another look after the integration tests restuls come back. if the results are good. we can merge this !

minikube-pr-bot · 2020-03-28T02:27:51Z

Error: running mkcmp: exit status 1

medyagh · 2020-03-28T05:12:15Z

Thank you very much for this PR, this will improve minikube service command !

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 4, 2020

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Mar 4, 2020

k8s-ci-robot requested review from josedonizetti and priyawadhwa March 4, 2020 06:31

prasadkatti force-pushed the populate_target_port_in_svc_list branch from eab12d9 to 720355b Compare March 4, 2020 06:35

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Mar 4, 2020

k8s-ci-robot assigned tstromberg Mar 4, 2020

populate target port in service list

9f41fd4

prasadkatti force-pushed the populate_target_port_in_svc_list branch from 720355b to 9f41fd4 Compare March 4, 2020 06:51

josedonizetti reviewed Mar 4, 2020

View reviewed changes

Add name/port instead of just the name for the target port

d19f27a

medyagh changed the title ~~service list: populate target port~~ wip: service list: populate target port Mar 4, 2020

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 4, 2020

fix minikube service tests

b7463fd

josedonizetti approved these changes Mar 5, 2020

View reviewed changes

prasadkatti changed the title ~~wip: service list: populate target port~~ service list: populate target port Mar 10, 2020

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 10, 2020

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 Mar 28, 2020

medyagh approved these changes Mar 28, 2020

View reviewed changes

medyagh merged commit da1d411 into kubernetes:master Mar 28, 2020

medyagh changed the title ~~service list: populate target port~~ service list cmd: display target port in the list Mar 28, 2020

medyagh changed the title ~~service list cmd: display target port in the list~~ service list cmd: display target port and name Mar 28, 2020

fmotrifork mentioned this pull request Apr 3, 2020

minikube 1.9.1 fishworks/fish-food#667

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

service list cmd: display target port and name #6879

service list cmd: display target port and name #6879

prasadkatti commented Mar 4, 2020 •

edited

Loading

k8s-ci-robot commented Mar 4, 2020

k8s-ci-robot commented Mar 4, 2020

k8s-ci-robot commented Mar 4, 2020

minikube-bot commented Mar 4, 2020

prasadkatti commented Mar 4, 2020

medyagh commented Mar 4, 2020

josedonizetti left a comment

josedonizetti Mar 4, 2020

medyagh Mar 4, 2020 •

edited

Loading

prasadkatti Mar 4, 2020

josedonizetti Mar 4, 2020 •

edited

Loading

prasadkatti Mar 4, 2020

medyagh commented Mar 4, 2020 •

edited

Loading

prasadkatti commented Mar 4, 2020

prasadkatti commented Mar 4, 2020

medyagh commented Mar 4, 2020 •

edited

Loading

codecov-io commented Mar 5, 2020

prasadkatti commented Mar 5, 2020

prasadkatti commented Mar 8, 2020

medyagh commented Mar 12, 2020

tstromberg commented Mar 21, 2020

prasadkatti commented Mar 27, 2020

medyagh commented Mar 28, 2020

medyagh commented Mar 28, 2020 •

edited

Loading

minikube-pr-bot commented Mar 28, 2020

medyagh commented Mar 28, 2020

service list cmd: display target port and name #6879

service list cmd: display target port and name #6879

Conversation

prasadkatti commented Mar 4, 2020 • edited Loading

Before

After

k8s-ci-robot commented Mar 4, 2020

k8s-ci-robot commented Mar 4, 2020

k8s-ci-robot commented Mar 4, 2020

minikube-bot commented Mar 4, 2020

prasadkatti commented Mar 4, 2020

medyagh commented Mar 4, 2020

josedonizetti left a comment

Choose a reason for hiding this comment

josedonizetti Mar 4, 2020

Choose a reason for hiding this comment

medyagh Mar 4, 2020 • edited Loading

Choose a reason for hiding this comment

prasadkatti Mar 4, 2020

Choose a reason for hiding this comment

josedonizetti Mar 4, 2020 • edited Loading

Choose a reason for hiding this comment

prasadkatti Mar 4, 2020

Choose a reason for hiding this comment

medyagh commented Mar 4, 2020 • edited Loading

prasadkatti commented Mar 4, 2020

prasadkatti commented Mar 4, 2020

medyagh commented Mar 4, 2020 • edited Loading

codecov-io commented Mar 5, 2020

Codecov Report

prasadkatti commented Mar 5, 2020

prasadkatti commented Mar 8, 2020

medyagh commented Mar 12, 2020

tstromberg commented Mar 21, 2020

prasadkatti commented Mar 27, 2020

medyagh commented Mar 28, 2020

medyagh commented Mar 28, 2020 • edited Loading

minikube-pr-bot commented Mar 28, 2020

medyagh commented Mar 28, 2020

prasadkatti commented Mar 4, 2020 •

edited

Loading

medyagh Mar 4, 2020 •

edited

Loading

josedonizetti Mar 4, 2020 •

edited

Loading

medyagh commented Mar 4, 2020 •

edited

Loading

medyagh commented Mar 4, 2020 •

edited

Loading

medyagh commented Mar 28, 2020 •

edited

Loading