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

odo url list -o json will not return empty list #1978

Closed
wants to merge 1 commit into from
Closed

odo url list -o json will not return empty list #1978

wants to merge 1 commit into from

Conversation

surajnarwade
Copy link
Contributor

@surajnarwade surajnarwade commented Aug 5, 2019 •
edited
Loading

What is the purpose of this change? What does it change?

odo url list -o json will not return empty list

Was the change discussed in an issue?

fixes #1893

How to test changes?

  • Create the URL using odo url create
  • Check odo url list -o json, it should give output

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Aug 5, 2019
@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

@surajnarwade surajnarwade changed the title will not return empty list odo url list -o json will not return empty list Aug 5, 2019
@surajnarwade surajnarwade marked this pull request as ready for review August 5, 2019 11:58
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Aug 5, 2019
girishramnani
girishramnani reviewed Aug 5, 2019
View reviewed changes
pkg/odo/cli/url/list.go
@@ -67,6 +69,23 @@ func (o *URLListOptions) Run() (err error) {
localUrls := o.localConfigInfo.GetUrl()

if log.IsJSON() {
if len(localUrls) != 0 {
for _, u := range localUrls {
Copy link
Contributor

Choose a reason for hiding this comment

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

might be a good idea to extract this out into a function, this is becoming a bit less readable in my opinion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe a more descriptive variable name than u

Copy link
Member

Choose a reason for hiding this comment

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

This whole logic should be probably part of url.List
The Url struct should have the Status field, similar to how it is done for Component to indicate if the Url is pushed or not.

@dharmit
Copy link
Member

dharmit commented Aug 6, 2019

@surajnarwade is there an open issue that this PR is addressing? Functionality LGTM 👍

$ odo url list -o json
{"kind":"List","apiVersion":"odo.openshift.io/v1alpha1","metadata":{},"items":null}

$ odo url list
Found the following URLs for component nodejs-nodejs-ex-kyvs in application app:
NAME                           IN CONFIG     URL                          PORT
nodejs-nodejs-ex-kyvs-8080     Present       <not created on cluster>     8080
To create URLs on the OpenShift Cluster, please use `odo push` 
nodejs-ex - master! $ cd $GOPATH/src/github.com/openshift/odo

# After building odo from this PR

$ odo url list -o json
{"kind":"List","apiVersion":"odo.openshift.io/v1alpha1","metadata":{},"items":[{"kind":"url","apiVersion":"odo.openshift.io/v1alpha1","metadata":{"name":"nodejs-nodejs-ex-kyvs-8080","creationTimestamp":null},"spec":{}}]}

$ odo url list
Found the following URLs for component nodejs-nodejs-ex-kyvs in application app:
NAME                           IN CONFIG     URL                          PORT
nodejs-nodejs-ex-kyvs-8080     Present       <not created on cluster>     8080
To create URLs on the OpenShift Cluster, please use `odo push

# After doing odo push

$ odo url list
Found the following URLs for component nodejs-nodejs-ex-kyvs in application app:
NAME                           IN CONFIG     URL                                                                      PORT
nodejs-nodejs-ex-kyvs-8080     Present       http://nodejs-nodejs-ex-kyvs-8080-app-myproject.192.168.42.48.nip.io     8080

$ odo url list -o json | jq .
{
  "kind": "List",
  "apiVersion": "odo.openshift.io/v1alpha1",
  "metadata": {},
  "items": [
    {
      "kind": "url",
      "apiVersion": "odo.openshift.io/v1alpha1",
      "metadata": {
        "name": "nodejs-nodejs-ex-kyvs-8080",
        "creationTimestamp": null
      },
      "spec": {
        "host": "nodejs-nodejs-ex-kyvs-8080-app-myproject.192.168.42.48.nip.io",
        "protocol": "http",
        "port": 8080
      }
    }
  ]
}

mik-dass
mik-dass reviewed Aug 7, 2019
View reviewed changes
pkg/odo/cli/url/list.go
@@ -67,6 +69,23 @@ func (o *URLListOptions) Run() (err error) {
localUrls := o.localConfigInfo.GetUrl()

if log.IsJSON() {
if len(localUrls) != 0 {
for _, u := range localUrls {
p, err := url.Exists(o.Client, u.Name, o.Component(), o.Application)
Copy link
Contributor

Choose a reason for hiding this comment

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

a more descriptive variable name than p

@kadel
Copy link
Member

kadel commented Aug 8, 2019

@surajnarwade Please add PR description or at least link to the issue?

@surajnarwade
Copy link
Contributor Author

@dharmit , I have updated the description with respective issue 😄

@surajnarwade
odo url list -o json will not return empty list
8f921ab 
`odo url list -o json` will not return empty list

fixes redhat-developer#1893
<!-- Please do Link issues here. -->

* Create the URL using `odo url create`
* Check `odo url list -o json`, it should give output
@kadel kadel mentioned this pull request Aug 22, 2019
Merged
@kadel
Copy link
Member

kadel commented Aug 22, 2019

replaced by #2034

/close

@openshift-ci-robot
Copy link
Collaborator

@kadel: Closed this PR.

In response to this:

replaced by #2034

/close

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.

@rm3l rm3l added the estimated-size/S (5-10) Rough sizing for Epics. Less then one sprint of work for one person label Jun 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mik-dass mik-dass mik-dass left review comments

@girishramnani girishramnani girishramnani left review comments

@kadel kadel Awaiting requested review from kadel

Assignees
No one assigned
Labels
estimated-size/S (5-10) Rough sizing for Epics. Less then one sprint of work for one person
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

odo url list -o json returns empty list for not pushed components with URLs
7 participants
@surajnarwade @openshift-ci-robot @dharmit @kadel @girishramnani @mik-dass @rm3l