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.

Sign up for GitHub

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

Jump to bottom

service list cmd: display target port and name #6879

Merged
merged 3 commits into from
Mar 28, 2020
Merged

service list cmd: display target port and name #6879

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9f41fd4
populate target port in service list
prasadkatti Mar 4, 2020
d19f27a
Add name/port instead of just the name for the target port
prasadkatti Mar 4, 2020
b7463fd
fix minikube service tests
prasadkatti Mar 5, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion cmd/minikube/cmd/service_list.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,14 +66,15 @@ var serviceListCmd = &cobra.Command{
if len(serviceURL.URLs) == 0 {
data = append(data, []string{serviceURL.Namespace, serviceURL.Name, "No node port"})
} else {
servicePortNames := strings.Join(serviceURL.PortNames, "\n")
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})
Copy link
Member

@josedonizetti josedonizetti Mar 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

}
}

Expand Down
8 changes: 8 additions & 0 deletions pkg/minikube/service/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"io"
"net/url"
"os"
"strconv"
"strings"
"text/template"
"time"
Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@medyagh medyagh Mar 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

} else {
m[port.TargetPort.IntVal] = strconv.Itoa(int(port.Port))
}

if port.NodePort > 0 {
var doc bytes.Buffer
err = t.Execute(&doc, struct {
Expand Down