Fixes odo url list for s2i components #3728
Conversation
openshift-ci-robot
added
the
kind/bug
mik-dass
force-pushed
the
fix_exp_url_list
branch
from
1da4336
to
28986fe
Compare
Codecov Report
@@ Coverage Diff @@
## master #3728 +/- ##
==========================================
- Coverage 44.93% 44.15% -0.79%
==========================================
Files 133 139 +6
Lines 12749 13345 +596
==========================================
+ Hits 5729 5892 +163
- Misses 6460 6874 +414
- Partials 560 579 +19
Continue to review full report at Codecov.
|
|
unit test on windows failed https://travis-ci.com/github/openshift/odo/jobs/370284958 with the python related error |
|
prow passed in 1 try |
pkg/odo/cli/url/list.go
Outdated
| "github.com/openshift/odo/pkg/odo/genericclioptions" | ||
| "github.com/openshift/odo/pkg/odo/util" | ||
| odoUtil "github.com/openshift/odo/pkg/odo/util" |
There was a problem hiding this comment.
better to use odoutil similar to how we do it in different packages. Instead of odoUtil
There was a problem hiding this comment.
I feel odoutil looks weird as it is not a camel case word. I think we should change all occurrences to odoUtil.
There was a problem hiding this comment.
Please refer to https://blog.golang.org/package-names
Package names should be short and sweet and not use camel case.
Example:
Package names
Good package names are short and clear. They are lower case, with no under_scores or mixedCaps. They are often simple nouns, such as:
time (provides functionality for measuring and displaying time)
list (implements a doubly linked list)
http (provides HTTP client and server implementations)
The style of names typical of another language might not be idiomatic in a Go program. Here are two examples of names that might be good style in other languages but do not fit well in Go:
computeServiceClient
priority_queue
A Go package may export several types and functions. For example, a compute package could export a Client type with methods for using the service as well as functions for partitioning a compute task across several clients.
There was a problem hiding this comment.
for example, below we import ktemplates, we do not use camelCase for that as kTemplates
openshift-ci-robot
added
the
lgtm
| @@ -364,5 +364,21 @@ var _ = Describe("odo devfile url command tests", func() { | |||
| output := helper.CmdShouldPass("oc", "get", "routes", "--namespace", namespace) | |||
| Expect(output).Should(ContainSubstring(url1)) | |||
| }) | |||
|
|
|||
| // remove once https://github.com/openshift/odo/issues/3550 is resolved | |||
| It("should list URLs for s2i components", func() { | |||
There was a problem hiding this comment.
+1, Integration test looks good to me.
|
/lgtm |
pkg/odo/cli/url/list.go
Outdated
| "github.com/openshift/odo/pkg/odo/genericclioptions" | ||
| "github.com/openshift/odo/pkg/odo/util" | ||
| odoUtil "github.com/openshift/odo/pkg/odo/util" |
There was a problem hiding this comment.
Please refer to https://blog.golang.org/package-names
Package names should be short and sweet and not use camel case.
Example:
Package names
Good package names are short and clear. They are lower case, with no under_scores or mixedCaps. They are often simple nouns, such as:
time (provides functionality for measuring and displaying time)
list (implements a doubly linked list)
http (provides HTTP client and server implementations)
The style of names typical of another language might not be idiomatic in a Go program. Here are two examples of names that might be good style in other languages but do not fit well in Go:
computeServiceClient
priority_queue
A Go package may export several types and functions. For example, a compute package could export a Client type with methods for using the service as well as functions for partitioning a compute task across several clients.
pkg/odo/cli/url/list.go
Outdated
| "github.com/openshift/odo/pkg/odo/genericclioptions" | ||
| "github.com/openshift/odo/pkg/odo/util" | ||
| odoUtil "github.com/openshift/odo/pkg/odo/util" |
There was a problem hiding this comment.
for example, below we import ktemplates, we do not use camelCase for that as kTemplates
openshift-ci-robot
removed
the
lgtm
|
@cdrage Thanks. I didn't know such a documentation regarding naming exists. I have pushed a commit which fixes this issue. |
/retest |
openshift-ci-robot
added
the
lgtm
/retest |
|
/approve |
|
[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 |
openshift-ci-robot
added
the
approved
|
/retest Please review the full test history for this PR and help us cut down flakes. |
3 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
|
/retest Please review the full test history for this PR and help us cut down flakes. |
What type of PR is this?
/kind bug
What does does this PR do / why we need it:
Fixes
odo url listfor s2i components with experimental mode on.Which issue(s) this PR fixes:
Fixes #3474
PR acceptance criteria:
Unit test
Integration test
Documentation
How to test changes / Special notes to the reviewer:
odo create nodejs --s2iodo url createfor both ingress and routesodo url listshould display both the URLs