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

List all urls even if they are only in local config or only in cluster #2034

Merged
merged 2 commits into from
Aug 30, 2019

Conversation

kadel
Copy link
Member

@kadel kadel commented Aug 22, 2019

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

This PR is adding State field into URL structs. Url can be in 3 different states:

  • Pushed: Url is present in local config and also on the cluster
  • Locally Deleted: Url is present on the cluster but not in the local config
  • Not Pushed: Url is present in the local config but not on the cluster

This is also reflected in the output

Was the change discussed in an issue?

fixes #1893
This PR replaces #1978

How to test changes?

Create some urls, and delete them without pushing.
You should see each url reporting correct state

odo create java:8
odo url create url0 --port 8080
odo url create url1 --port 8443
odo push
odo url delete url1
odo url create url2 --port 8778

# now you should see each url in a different state
odo url list
▶ odo url list
Found the following URLs for component java-spring-boot-yelj in application app:
NAME                           STATE               URL                                                                                       PORT
java-spring-boot-yelj-8080     Pushed              http://java-spring-boot-yelj-8080-app-tkmyproject.apps.tkral.devcluster.openshift.com     8080
java-spring-boot-yelj-8443     Locally Deleted     http://java-spring-boot-yelj-8443-app-tkmyproject.apps.tkral.devcluster.openshift.com     8443
java-spring-boot-yelj-8778     Not Pushed          ://


There are local changes. Please run 'odo push'.%
▶ odo url list -o json | jq .
{
  "kind": "List",
  "apiVersion": "odo.openshift.io/v1alpha1",
  "metadata": {},
  "items": [
    {
      "kind": "url",
      "apiVersion": "odo.openshift.io/v1alpha1",
      "metadata": {
        "name": "java-spring-boot-yelj-8080",
        "creationTimestamp": null
      },
      "spec": {
        "host": "java-spring-boot-yelj-8080-app-tkmyproject.apps.tkral.devcluster.openshift.com",
        "protocol": "http",
        "port": 8080
      },
      "status": {
        "state": "Pushed"
      }
    },
    {
      "kind": "url",
      "apiVersion": "odo.openshift.io/v1alpha1",
      "metadata": {
        "name": "java-spring-boot-yelj-8443",
        "creationTimestamp": null
      },
      "spec": {
        "host": "java-spring-boot-yelj-8443-app-tkmyproject.apps.tkral.devcluster.openshift.com",
        "protocol": "http",
        "port": 8443
      },
      "status": {
        "state": "Locally Deleted"
      }
    },
    {
      "kind": "url",
      "apiVersion": "odo.openshift.io/v1alpha1",
      "metadata": {
        "name": "java-spring-boot-yelj-8778",
        "creationTimestamp": null
      },
      "spec": {
        "port": 8778
      },
      "status": {
        "state": "Not Pushed"
      }
    }
  ]
}

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.

  • creationTimestamp is empty in the metadata section of json output. Is it not possible to set it? Is it not of use to the dev? Is it there for future purpose? If it's not supposed to have the value, why have it in the metadata?
  • Code looks good, but can use a few comments.

pkg/odo/cli/url/list.go Show resolved Hide resolved
pkg/odo/cli/url/list.go Outdated Show resolved Hide resolved
pkg/url/url.go Outdated Show resolved Hide resolved
pkg/url/url.go Show resolved Hide resolved
@kadel
Copy link
Member Author

kadel commented Aug 26, 2019

  • creationTimestamp is empty in the metadata section of json output. Is it not possible to set it? Is it not of use to the dev? Is it there for future purpose? If it's not supposed to have the value, why have it in the metadata?

That comes from reusing k8s metav1.ObjectMeta, currently we have it empty everywhere, and we can't get rid of it, unless we create our own ObjectMeta In future we might start using it, but for now, it is like this everywhere.

@kadel
Copy link
Member Author

kadel commented Aug 26, 2019

/retest


error: could not run steps: step root failed: could not wait for build: the build root failed after 18s with reason PullBuilderImageFailed: Failed pulling builder image.
 Pulling image registry.svc.ci.openshift.org/openshift/release:golang-1.11 ...
error: build error: failed to pull image: received unexpected HTTP status: 500 Internal Server Error

@kadel
Copy link
Member Author

kadel commented Aug 27, 2019

/retest

2019/08/26 15:52:58 Submitted failure event to sentry (id=e9e89ca9e8054c18a73209af210c988b)
error: could not run steps: step root failed: could not wait for build: the build root failed after 42s with reason PullBuilderImageFailed: Failed pulling builder image.
 Pulling image registry.svc.ci.openshift.org/openshift/release:golang-1.11 ...
error: build error: failed to pull image: received unexpected HTTP status: 500 Internal Server Error

Copy link
Contributor

@amitkrout amitkrout left a comment

Choose a reason for hiding this comment

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

Integration test script update looks good to me. @kadel However UTs coverage is not done properly for newly added function.

@kadel
Copy link
Member Author

kadel commented Aug 29, 2019

Integration test script update looks good to me. @kadel However UTs coverage is not done properly for newly added function.

Due to the current structure, it is almost impossible to write a unit test for url.List function :-( This will require big config package refactoring. All the scenarios should be covered in integration test.

Copy link
Member

@cdrage cdrage left a comment

Choose a reason for hiding this comment

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

Works and code LGTM! I don't see any issues as well as -o json works well.

~/nodejs-ex  master ✗                                                                                                                                                                                                                                                  317d ⚑ ◒  
▶ odo url list
Found the following URLs for component nodejs-nodejs-ex-sgwk in application app:
NAME                           STATE      URL                                                                                PORT
nodejs-nodejs-ex-sgwk-8080     Pushed     [REDACTED]     8080
url0                           Pushed     [REDACTED]                           8080

~/nodejs-ex  master ✗                                                                                                                                                                                                                                                  317d ⚑ ◒  
▶ odo url delete url0
? Are you sure you want to delete the url url0 Yes
 ✓  URL url0 removed from the config file
To delete URL on the OpenShift Cluster, please use `odo push`

~/nodejs-ex  master ✗                                                                                                                                                                                                                                                  317d ⚑ ◒  
▶ odo url list
Found the following URLs for component nodejs-nodejs-ex-sgwk in application app:
NAME                           STATE               URL                                                                                PORT
nodejs-nodejs-ex-sgwk-8080     Pushed              [REDACTED]     8080
url0                           Locally Deleted     [REDACTED]                           8080

There are local changes. Please run 'odo push'.

/approve

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cdrage

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 Aug 29, 2019
@dharmit
Copy link
Member

dharmit commented Aug 30, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Aug 30, 2019
@openshift-merge-robot openshift-merge-robot merged commit 4dfff42 into redhat-developer:master Aug 30, 2019
cdrage added a commit to cdrage/odo that referenced this pull request Sep 3, 2019
- Lists all URLs even if they are undeployed or not
(redhat-developer#2034)

- Added json output for `odo project delete -o json`
(redhat-developer#2037)

- Fixed service integration test that was previously failing
(redhat-developer#2022)

- `odo push` will now only push changed files
(redhat-developer#2030)
- We now use relative paths within the file indexer
(redhat-developer#2003)
- You can now create list and edit services using --app and --project
(redhat-developer#2001)
- Deleted files will now propagate to the OpenShift container
(redhat-developer#1999)
- Added `odo service create --context` functionality
(redhat-developer#1997)
- Making cross-compile independant of gox vendor package
(redhat-developer#2047)
- `odo-supervisord-image` has been renamed to `odo-init-image`
(redhat-developer#2027)
- Releases now use .tar.gz (redhat-developer#2009)
- If there is an error creating a service, it will fail quicker
(redhat-developer#2008)

- We now have a Google Group!
(redhat-developer#2007)
- Added documentation on how to manage environment
variables(redhat-developer#2026)
- Badges added to the README
(redhat-developer#2060)
- Updated documentation on uninstallation
(redhat-developer#2053)
- Added documentation for default parameters
(redhat-developer#2038)
- Minor update to help output
(redhat-developer#2006)
- Updated documentation regarding bootstrapper image
(redhat-developer#1991)
@cdrage cdrage mentioned this pull request Sep 3, 2019
openshift-merge-robot pushed a commit that referenced this pull request Sep 3, 2019
- Lists all URLs even if they are undeployed or not
(#2034)

- Added json output for `odo project delete -o json`
(#2037)

- Fixed service integration test that was previously failing
(#2022)

- `odo push` will now only push changed files
(#2030)
- We now use relative paths within the file indexer
(#2003)
- You can now create list and edit services using --app and --project
(#2001)
- Deleted files will now propagate to the OpenShift container
(#1999)
- Added `odo service create --context` functionality
(#1997)
- Making cross-compile independant of gox vendor package
(#2047)
- `odo-supervisord-image` has been renamed to `odo-init-image`
(#2027)
- Releases now use .tar.gz (#2009)
- If there is an error creating a service, it will fail quicker
(#2008)

- We now have a Google Group!
(#2007)
- Added documentation on how to manage environment
variables(#2026)
- Badges added to the README
(#2060)
- Updated documentation on uninstallation
(#2053)
- Added documentation for default parameters
(#2038)
- Minor update to help output
(#2006)
- Updated documentation regarding bootstrapper image
(#1991)
@rm3l rm3l added estimated-size/L (20-40) Rough sizing for Epics. About 2 sprints of work for a person. and removed size/L labels Jun 18, 2023
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. estimated-size/L (20-40) Rough sizing for Epics. About 2 sprints of work for a person. 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.

odo url list -o json returns empty list for not pushed components with URLs
7 participants