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

Adds secure field to url list table ouput and json output. #2817

Merged

Conversation

mik-dass
Copy link
Contributor

@mik-dass mik-dass commented Apr 6, 2020

What type of PR is this?

/kind feature

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

Adds information about the url being a secure one or not in odo url list normal and json output.

Which issue(s) this PR fixes:

Fixes #2775

How to test changes / Special notes to the reviewer:

  • create a normal odo component with secure url and execute odo url list and also json version of it.
  • create a devfile odo component with secure url and execute odo url list and also json version of it.

The outputs should contain information about the url being a secure one or not.

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

// are there changes between local and cluster states?
outOfSync := false
for _, i := range localUrls {
var present bool
for _, u := range urls.Items {
if i.Name == u.Name {
fmt.Fprintln(tabWriterURL, u.Name, "\t", url.GetURLString(url.GetProtocol(routev1.Route{}, u), "", u.Spec.Rules[0].Host), "\t", u.Spec.Rules[0].IngressRuleValue.HTTP.Paths[0].Backend.ServicePort.IntVal)
fmt.Fprintln(tabWriterURL, u.Name, "\t", url.GetURLString(url.GetProtocol(routev1.Route{}, u), "", u.Spec.Rules[0].Host), "\t", u.Spec.Rules[0].IngressRuleValue.HTTP.Paths[0].Backend.ServicePort.IntVal, "\t", u.Spec.TLS != nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

can we make this more readable? just top of my head maybe extract url.GetURLString(url.GetProtocol(routev1.Route{}, u), "", u.Spec.Rules[0].Host) into a helper?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

url.GetURLString(url.GetProtocol(routev1.Route{}, u), "", u.Spec.Rules[0].Host)

But url.GetURLString() is already a helper function.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am ok if you feel we cannot make this more clean.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried certain things but it was making things a bit complicated and weird for me.

@girishramnani
Copy link
Contributor

/retest

@girishramnani
Copy link
Contributor

/approve

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: girishramnani

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 Apr 8, 2020
@dharmit
Copy link
Member

dharmit commented Apr 9, 2020

For a URL that's not set to be secure, is it expected that we don't see anything in json output?

$ odo url list
Found the following URLs for component nodejs-nodejs-ex-zdgz in application app:
NAME     STATE      URL                                            PORT     SECURE
url1     Pushed     http://url1-app-myproject.apps-crc.testing     8080     false

$ odo url list -o json
{
    "kind": "List",
    "apiVersion": "odo.openshift.io/v1alpha1",
    "metadata": {},
    "items": [
        {
            "kind": "url",
            "apiVersion": "odo.openshift.io/v1alpha1",
            "metadata": {
                "name": "url1",
                "creationTimestamp": null
            },
            "spec": {
                "host": "url1-app-myproject.apps-crc.testing",
                "protocol": "http",
                "port": 8080
            },
            "status": {
                "state": "Pushed"
            }
        }
    ]
}

@mik-dass
Copy link
Contributor Author

mik-dass commented Apr 9, 2020

For a URL that's not set to be secure, is it expected that we don't see anything in json output?

We are using json:"secure,omitempty". So it gets removed during json formatting when it is false. @dharmit @kadel @girishramnani Should I keep it?

@dharmit
Copy link
Member

dharmit commented Apr 9, 2020

I'd vote to keep it. Explicit is better than implicit. 😉

Let's wait for others to chime in. 😄

@mik-dass
Copy link
Contributor Author

mik-dass commented Apr 9, 2020

I'd vote to keep it. Explicit is better than implicit. wink

Added back. Please have a look again :)

@amitkrout
Copy link
Contributor

We are using json:"secure,omitempty". So it gets removed during json formatting when it is false. @dharmit @kadel @girishramnani Should I keep it?

Test looks good to me. I won't mind if you guys finalize the concern and apply lgtm label.

@codecov
Copy link

codecov bot commented Apr 9, 2020

Codecov Report

Merging #2817 into master will decrease coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2817      +/-   ##
==========================================
- Coverage   43.70%   43.68%   -0.03%     
==========================================
  Files          97       97              
  Lines        8783     8801      +18     
==========================================
+ Hits         3839     3845       +6     
- Misses       4580     4593      +13     
+ Partials      364      363       -1     
Impacted Files Coverage Δ
pkg/url/url.go 28.07% <100.00%> (ø)
pkg/util/util.go 65.40% <0.00%> (-1.70%) ⬇️
pkg/application/application.go 40.00% <0.00%> (-1.27%) ⬇️
pkg/odo/util/cmdutils.go 1.69% <0.00%> (ø)
pkg/occlient/occlient.go 52.73% <0.00%> (+0.03%) ⬆️
pkg/component/component.go 24.38% <0.00%> (+0.59%) ⬆️

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 3032491...f5610e1. Read the comment docs.

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.

Functionality wise works for me. Code looks good. Thanks for addressing the false case @mik-dass 👍

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Apr 14, 2020
@openshift-bot
Copy link

/retest

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

1 similar comment
@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 aa410cf into redhat-developer:master Apr 14, 2020
@mik-dass mik-dass deleted the secure_display branch April 15, 2020 05:18
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/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation 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'/'odo url list -o json' does not include information to distinguish secure and not secure URLs
7 participants