Add support for url describe command #2694

jichenjc · 2020-03-10T02:34:25Z

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespace from that line:

/kind api-change
/kind bug
/kind cleanup
/kind deprecation
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake
/kind code-refactoring

What does does this PR do / why we need it:

implemented this: I certainly can do further change on the output format, just submit
this for some comments

[root@xyimgr77 frontend]# ../odo url list
Found the following URLs for component ubi-frontend-gmhz in application app:
NAME                       STATE          URL     PORT
ubi-frontend-gmhz-8080     Not Pushed     ://     8080

There are local changes. Please run 'odo push'.
[root@xyimgr77 frontend]# ../odo url describe
 ✗  url  not found
[root@xyimgr77 frontend]# ../odo url describe abc
 ✗  url abc not found
[root@xyimgr77 frontend]# ../odo url describe ubi-frontend-gmhz-8080
NAME                       STATE          URL     PORT
ubi-frontend-gmhz-8080     Not Pushed     ://     8080

There are local changes. Please run 'odo push'.
[root@xyimgr77 frontend]#

Which issue(s) this PR fixes:

Fixes #?

How to test changes / Special notes to the reviewer:

openshift-ci-robot · 2020-03-10T02:34:39Z

Hi @jichenjc. Thanks for your PR.

I'm waiting for a openshift 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.

kadel · 2020-03-11T17:04:09Z

/ok-to-test

mik-dass · 2020-03-12T05:46:48Z

@jichenjc Thanks for the PR. Please edit the description and link the issue with this PR. Also please add some steps for testing the PR.

pkg/url/url.go

mik-dass · 2020-03-12T06:29:17Z

pkg/odo/cli/url/describe.go

+	urls, err := url.List(o.Client, o.localConfigInfo, o.Component(), o.Application)
+	if err != nil {
+		return err
+	}
+	u, err := urls.Get(o.url)
+	if err != nil {
+		return err
+	}


Since url describe can take only one URL name as a parameter input, I don't think we need url.List() here. We have faced slowness because of listing when we didn't need it. Please write a generic Get function for URL and use the client to make a get call for a route. Also to find the state, please use the local Config.

got it , will double check how to do this

@mik-dass
um.. after double check , seems we have something like
clusterURL := getMachineReadableFormat(r)
which means we can't search in etcd directly, we need get back the data then do some transform and
this of course need list ,

I can change the function to Get() the compare in the new function but still need list
so don't know how much improvement we get can... so, still a O(n) search , not O(1)

No you don't need a list. First write getRoute() function in occlient.go like https://github.com/openshift/odo/blob/master/pkg/occlient/occlient.go#L484 which returns the found route. Then write a Get() (don't use the existing one, it requires a list) function, call the getRoute() in occlient.go and transform the returned route to machine readable format using getMachineReadableFormat().

ok, get your point, I will try the way you suggested, thanks!

pkg/odo/cli/url/describe.go

mik-dass · 2020-03-12T06:37:32Z

pkg/odo/util/cmdutils.go

@@ -136,7 +136,7 @@ func PrintComponentInfo(client *occlient.Client, currentComponentName string, co

 			// Gather the output
 			for _, componentURL := range componentDesc.Spec.URL {
-				url := urls.Get(componentURL)
+				url, _ := urls.Get(componentURL)


please don't ignore the error. If the function returns a error, this might cause a panic as the returned URL will be empty.

got it , thanks

pkg/odo/cli/url/describe.go

mik-dass · 2020-03-12T06:42:55Z

Also please add some integration test for this feature

jichenjc · 2020-03-13T13:52:28Z

/retest

codecov · 2020-03-16T09:54:31Z

Codecov Report

Merging #2694 into master will increase coverage by 0.29%.
The diff coverage is 18.51%.

@@            Coverage Diff             @@
##           master    #2694      +/-   ##
==========================================
+ Coverage   43.13%   43.43%   +0.29%     
==========================================
  Files          77       86       +9     
  Lines        7314     7978     +664     
==========================================
+ Hits         3155     3465     +310     
- Misses       3855     4173     +318     
- Partials      304      340      +36

Impacted Files	Coverage Δ
pkg/url/url.go	`39.61% <0%> (-6.61%)`	⬇️
pkg/occlient/occlient.go	`52.55% <100%> (+2.95%)`	⬆️
pkg/odo/cli/catalog/util/util.go	`40.47% <0%> (-18.15%)`	⬇️
pkg/kclient/deployments.go	`89.65% <0%> (-10.35%)`	⬇️
pkg/kclient/generators.go	`91.25% <0%> (-8.75%)`	⬇️
pkg/kclient/kclient.go	`26.92% <0%> (-3.52%)`	⬇️
pkg/catalog/catalog.go	`51.77% <0%> (-2.19%)`	⬇️
pkg/kclient/volumes.go	`93.22% <0%> (-1.66%)`	⬇️
pkg/util/util.go	`65.73% <0%> (-0.93%)`	⬇️
pkg/kclient/pods.go	`41.42% <0%> (-0.61%)`	⬇️
... and 32 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8e6116a...8a39bd6. Read the comment docs.

pkg/url/url.go

mik-dass · 2020-03-18T10:31:33Z

tests/integration/cmd_url_test.go

@@ -88,6 +88,21 @@ var _ = Describe("odo url command tests", func() {
 		})
 	})

+	Context("Describing urls", func() {
+		It("should describe appropriate URLs and push message", func() {


please add some more cases with the various pushed states or maybe even add them here itself

pkg/url/url.go

pkg/odo/cli/url/describe.go

kadel · 2020-03-20T09:25:02Z

This might not be that useful right now, as it is not providing any additional information compared to odo url list but I can see that in the future odo can provide more detailed information about url.

👍
/approve

openshift-ci-robot · 2020-03-20T09:25:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kadel

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:

~~OWNERS~~ [kadel]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

amitkrout · 2020-03-21T10:21:56Z

Integration test looks good to me
/lgtm

openshift-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. Required by Prow. size/L labels Mar 10, 2020

openshift-ci-robot requested review from elsony and johnmcollier March 10, 2020 02:35

openshift-ci-robot added the kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation label Mar 10, 2020

openshift-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. Required by Prow. labels Mar 11, 2020