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.

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

nit: dont show url as :// in odo url list #4111

Merged

Conversation

girishramnani
Copy link
Contributor

What type of PR is this?

Uncomment only one /kind line, and delete the rest.
For example, > /kind bug would simply become: /kind bug

/kind bug

What does does this PR do / why we need it:
before doing odo push if there is a url present then we show it as :// and that's confusing. Resolved it by making it a blank until user pushes.

Which issue(s) this PR fixes:

Fixes #4109

PR acceptance criteria:

How to test changes / Special notes to the reviewer:
way to reproduce

odo create nodejs
odo url create --port 3000
odo url list

Output:

Found the following URLs for component nodejs
NAME STATE URL PORT SECURE KIND
nodejs-3000 Not Pushed :// 3000 false route

After fix the url would be blank

@openshift-ci-robot openshift-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Oct 12, 2020
@mik-dass
Copy link
Contributor

mik-dass commented Oct 12, 2020

[mrinaldas@localhost project]$ odo create nodejs
Validation
 ✓  Checking devfile existence [38692ns]
 ✓  Creating a devfile component from registry: odo-V2-Registry [58716ns]
 ✓  Validating devfile component [193853ns]

Please use `odo push` command to create the component with source deployed
[mrinaldas@localhost project]$ odo url  list
Found the following URLs for component nodejs
NAME          STATE          URL     PORT     SECURE     KIND
http-3000     Not Pushed     ://     3000     false      route
There are local changes. Please run 'odo push'.
[mrinaldas@localhost project]$ odo url create example
 ✓  URL example created for component: nodejs

To apply the URL configuration changes, please use `odo push`
[mrinaldas@localhost project]$ odo url  list
Found the following URLs for component nodejs
NAME          STATE          URL     PORT     SECURE     KIND
example       Not Pushed     ://     3000     false      route
http-3000     Not Pushed     ://     3000     false      route

Not working for me on a Openshift cluster

It works for s2i components though

[mrinaldas@localhost project]$ odo create nodejs --s2i
Validation
 ✓  Validating component [1s]

Please use `odo push` command to create the component with source deployed
[mrinaldas@localhost project]$ odo url  list
aFound the following URLs for component nodejs-project-cefh in application app:
NAME        STATE          URL     PORT     SECURE
example     Not Pushed             8080     false

@dharmit
Copy link
Member

dharmit commented Oct 12, 2020

Not working for me on a Openshift cluster

Same here on CRC.

$ odo url create url1 --port 8080
 ✓  URL url1 created for component: [node-1](https://issues.redhat.com/browse/node-1)

To apply the URL configuration changes, please use `odo push`


$ odo url list
Found the following URLs for component [node-1](https://issues.redhat.com/browse/node-1)
NAME     STATE          URL     PORT     SECURE     KIND
url1     Not Pushed     ://     8080     false      route
There are local changes. Please run 'odo push'.

Copy link
Member

@dharmit dharmit left a comment

Choose a reason for hiding this comment

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

$ odo url list                                                     
Found the following URLs for component nodetodo
NAME          STATE          URL     PORT     SECURE     KIND
http-3000     Not Pushed             3000     false      route

Works for me!
/approve

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dharmit

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:

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label Oct 14, 2020
@@ -111,9 +112,11 @@ var _ = Describe("odo devfile url command tests", func() {

helper.CopyExample(filepath.Join("source", "devfiles", "nodejs", "project"), commonVar.Context)
helper.CopyExampleDevFile(filepath.Join("source", "devfiles", "nodejs", "devfile.yaml"), filepath.Join(commonVar.Context, "devfile.yaml"))

helper.CmdShouldPass("odo", "url", "create", url1, "--host", host, "--ingress")
// we check the default url
stdout := helper.CmdShouldPass("odo", "url", "list")
Copy link
Contributor

Choose a reason for hiding this comment

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

This will fail on a kubernetes cluster

[odo]  ✗  no URLs found for component rgsrbu. Refer `odo url create -h` to add one
Deleting project: gqpnssmcal
Running kubectl with args [kubectl delete namespaces gqpnssmcal]
[kubectl] namespace "gqpnssmcal" deleted
Setting current dir to: /home/travis/gopath/src/github.com/openshift/odo/tests/integration/devfile

https://travis-ci.com/github/openshift/odo/jobs/399332372#L1459

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved

@codecov
Copy link

codecov bot commented Oct 14, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@2a68f57). Click here to learn what that means.
The diff coverage is n/a.

@mohammedzee1000
Copy link
Contributor

/refresh

@girishramnani
Copy link
Contributor Author

Still cannot see tidy on this PR

@girishramnani
Copy link
Contributor Author

/retest

1 similar comment
@girishramnani
Copy link
Contributor Author

/retest

Copy link
Contributor

@mik-dass mik-dass left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Oct 19, 2020
@girishramnani
Copy link
Contributor Author

/retest

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 460dacc into redhat-developer:master Oct 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. Required by Prow.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nit: odo url list shows :// before it is pushed
7 participants