Refactor to remove opening browser and just return url(s) #5718

nanikjava · 2019-10-24T11:25:17Z

The WaitAndMaybeOpenService(..) method allow user to open the service
url in a browser when found, this create issue during testing as it's
opening browser and consuming resources.

The fix is to introduce a boolean flag allowing the caller to specify
whether to just print out the url or open a browser window

k8s-ci-robot · 2019-10-24T11:25:25Z

Hi @nanikjava. 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.

minikube-bot · 2019-10-24T11:26:03Z

Can one of the admins verify this patch?

codecov-io · 2019-10-24T11:31:09Z

Codecov Report

Merging #5718 into master will decrease coverage by 0.04%.
The diff coverage is 22.22%.

@@            Coverage Diff             @@
##           master    #5718      +/-   ##
==========================================
- Coverage   37.83%   37.78%   -0.05%     
==========================================
  Files         106      106              
  Lines        7773     7780       +7     
==========================================
- Hits         2941     2940       -1     
- Misses       4452     4461       +9     
+ Partials      380      379       -1

Impacted Files	Coverage Δ
pkg/util/utils.go	`37.89% <0%> (-1.24%)`	⬇️
cmd/minikube/cmd/service.go	`23.33% <0%> (-2.6%)`	⬇️
cmd/minikube/cmd/config/open.go	`14% <0%> (-1.22%)`	⬇️
pkg/minikube/service/service.go	`76.88% <66.66%> (+0.69%)`	⬆️

tstromberg · 2019-10-24T20:16:30Z

pkg/minikube/service/service.go

@@ -267,7 +267,7 @@ func PrintServiceList(writer io.Writer, data [][]string) {

 // WaitAndMaybeOpenService waits for a service, and opens it when running
 func WaitAndMaybeOpenService(api libmachine.API, namespace string, service string, urlTemplate *template.Template, urlMode bool, https bool,


WaitAndMaybeOpenService already has too many arguments, let's not add another.

How about removing the browser.OpenURL call altogether, and leaving that up to cmd/service.go to handle. Then this function could be renamed to a more definite WaitForService, and be more testable.

Agree with tstromberg

Cool. Will make the changes

tstromberg

This refactor looks great. Just a few minor suggestions for clarity. Thank you for doing this.

tstromberg · 2019-10-26T17:37:35Z

pkg/minikube/service/service.go

-// WaitAndMaybeOpenService waits for a service, and opens it when running
-func WaitAndMaybeOpenService(api libmachine.API, namespace string, service string, urlTemplate *template.Template, urlMode bool, https bool,
-	wait int, interval int) error {
+// WaitForService waits for a service, and opens it when running


Make comment accurate again

This method no longer opens a URL.

Comment has been updated to reflect returning url instead of opening in browser. PR title has been modified too.

tstromberg · 2019-10-26T17:38:37Z

cmd/minikube/cmd/service.go

 			serviceURLTemplate, serviceURLMode, https, wait, interval)
 		if err != nil {
 			exit.WithError("Error opening service", err)
 		}
+
+		if len(urlString) != 0 {


WaitAndMaybeOpenService had the following logic for opening a URL:

if urlMode || !isHTTPSchemedURL {

Make sure your PR preserves the same behavior as before.

The code that you are referring to is inside pkg/minikube/service/service.go and the only changes made was to send back the array of urls

tstromberg · 2019-10-26T17:39:48Z

pkg/minikube/service/service.go

+func WaitForService(api libmachine.API, namespace string, service string, urlTemplate *template.Template, urlMode bool, https bool,
+	wait int, interval int) (string, error) {
+
+	var urlString string


Just url is fine as a variable name. It generally isn't useful to repeat the type of an object in the object name.

See also https://github.com/golang/go/wiki/CodeReviewComments#variable-names

tstromberg · 2019-10-26T17:42:40Z

pkg/minikube/service/service_test.go

@@ -903,12 +903,12 @@ func TestWaitAndMaybeOpenService(t *testing.T) {
 				servicesMap:  serviceNamespaces,
 				endpointsMap: endpointNamespaces,
 			}
-			err := WaitAndMaybeOpenService(test.api, test.namespace, test.service, defaultTemplate, test.urlMode, test.https, 1, 0)
+			_, err := WaitForService(test.api, test.namespace, test.service, defaultTemplate, test.urlMode, test.https, 1, 0)


This returns a URL now - please test the returned value of it.

@tstromberg Want to bounce some thoughts. In master branch the current implementation is

for _, bareURLString := range serviceURL.URLs { urlString, isHTTPSchemedURL := OptionallyHTTPSFormattedURLString(bareURLString, https) if urlMode || !isHTTPSchemedURL { out.T(out.Empty, urlString) } else { out.T(out.Celebrate, "Opening kubernetes service {{.namespace_name}}/{{.service_name}} in default browser...", out.V{"namespace_name": namespace, "service_name": service}) if err := browser.OpenURL(urlString); err != nil { out.ErrT(out.Empty, "browser failed to open url: {{.error}}", out.V{"error": err}) } } }

which means that there is possibility that there are more than 1 url, so is it correct to also
implement string array as the returned value ?

After going through the code and the functionality the function will have to return array of url. So have made changes to both the code and test cases.

tstromberg · 2019-10-26T17:45:26Z

pkg/util/utils.go

@@ -200,3 +202,9 @@ func ConcatStrings(src []string, prefix string, postfix string) []string {
 	}
 	return ret
 }
+
+func OpenBrowser(urlString string) {


This function doesn't add ta lot of value, so just remove it and inline the code instead. The only thing really duplicated is the error message.

Also, we're trying to get rid of unscoped packages like the util package.

tstromberg · 2019-10-26T17:46:52Z

cmd/minikube/cmd/service.go

+
+		if len(urlString) != 0 {
+			out.T(out.Celebrate, "Opening kubernetes service  {{.namespace_name}}/{{.service_name}} in default browser...", out.V{"namespace_name": namespace, "service_name": svc})
+			util.OpenBrowser(urlString)


If opening the browser fails, call exit.WithError

sharifelgamal · 2019-10-28T19:55:18Z

/ok-to-test

tstromberg

Please fix PR title and description to reflect the most recent changes. Looks mergeable otherwise.

tstromberg · 2019-10-29T02:11:12Z

pkg/minikube/service/service.go

-// WaitAndMaybeOpenService waits for a service, and opens it when running
-func WaitAndMaybeOpenService(api libmachine.API, namespace string, service string, urlTemplate *template.Template, urlMode bool, https bool,
-	wait int, interval int) error {
+// WaitForService waits for a service, and opens it when running


This method no longer opens a URL.

The WaitAndMaybeOpenService(..) method allow user to open the service url in a browser when found, this create issue during testing as it's opening browser and consuming resources. The fix is to introduce a boolean flag allowing the caller to specify whether to just print out the url or open a browser window

k8s-ci-robot · 2019-10-29T02:24:31Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

tstromberg · 2019-10-29T20:51:46Z

Thank you for sticking through with the review process!

nanikjava · 2019-10-29T20:57:25Z

Thank you for sticking through the review process!

No worries. Thanks for being patience in reviewing my PRs, still learning :)

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 24, 2019

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Oct 24, 2019

k8s-ci-robot requested review from afbjorklund and RA489 October 24, 2019 11:25

tstromberg reviewed Oct 24, 2019

View reviewed changes

nanikjava force-pushed the f-fix-test-disable-browser-test branch from f4e2c7b to 2a2ee37 Compare October 26, 2019 06:00

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. 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 Oct 26, 2019

nanikjava force-pushed the f-fix-test-disable-browser-test branch from 2a2ee37 to 8e36c45 Compare October 26, 2019 06:07

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 26, 2019

tstromberg suggested changes Oct 26, 2019

View reviewed changes

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 Oct 28, 2019

nanikjava force-pushed the f-fix-test-disable-browser-test branch from 8e36c45 to 8a88e86 Compare October 28, 2019 20:39

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 28, 2019

tstromberg suggested changes Oct 29, 2019

View reviewed changes

nanikjava force-pushed the f-fix-test-disable-browser-test branch from 8a88e86 to 994d093 Compare October 29, 2019 02:24

nanikjava changed the title ~~Add boolean to disable opening browser during test~~ Refactor to remove opening browser and just return url(s) Oct 29, 2019

tstromberg approved these changes Oct 29, 2019

View reviewed changes

tstromberg merged commit 74a5d47 into kubernetes:master Oct 29, 2019

This was referenced Oct 30, 2019

minikube service opens in browser when it should not #5789

Closed

service: fix --url mode, add integration tests #5790

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor to remove opening browser and just return url(s) #5718

Refactor to remove opening browser and just return url(s) #5718

nanikjava commented Oct 24, 2019

k8s-ci-robot commented Oct 24, 2019

minikube-bot commented Oct 24, 2019

codecov-io commented Oct 24, 2019 •

edited

Loading

tstromberg Oct 24, 2019

medyagh Oct 24, 2019

nanikjava Oct 24, 2019

tstromberg left a comment

tstromberg Oct 26, 2019

tstromberg Oct 29, 2019

nanikjava Oct 29, 2019

tstromberg Oct 26, 2019

nanikjava Oct 28, 2019

tstromberg Oct 26, 2019

tstromberg Oct 26, 2019

nanikjava Oct 27, 2019

nanikjava Oct 28, 2019

tstromberg Oct 26, 2019

tstromberg Oct 26, 2019

sharifelgamal commented Oct 28, 2019

tstromberg left a comment

tstromberg Oct 29, 2019

k8s-ci-robot commented Oct 29, 2019

tstromberg commented Oct 29, 2019 •

edited

Loading

nanikjava commented Oct 29, 2019

		@@ -267,7 +267,7 @@ func PrintServiceList(writer io.Writer, data [][]string) {

		// WaitAndMaybeOpenService waits for a service, and opens it when running
		func WaitAndMaybeOpenService(api libmachine.API, namespace string, service string, urlTemplate *template.Template, urlMode bool, https bool,

Refactor to remove opening browser and just return url(s) #5718

Refactor to remove opening browser and just return url(s) #5718

Conversation

nanikjava commented Oct 24, 2019

k8s-ci-robot commented Oct 24, 2019

minikube-bot commented Oct 24, 2019

codecov-io commented Oct 24, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tstromberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharifelgamal commented Oct 28, 2019

tstromberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 29, 2019

tstromberg commented Oct 29, 2019 • edited Loading

nanikjava commented Oct 29, 2019

codecov-io commented Oct 24, 2019 •

edited

Loading

tstromberg commented Oct 29, 2019 •

edited

Loading