remove duplicate list and refactor #4177

girishramnani · 2020-10-30T06:00:48Z

What type of PR is this?
/kind bug
/kind cleanup

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

resolve odo list printing devfile components as s2i components
refactor the code to not use Kclient directly in the cli
refactor the code to not use Deployment struct ( structs from the lowest ) layer in the cli layer

Which issue(s) this PR fixes:

Fixes #4144

PR acceptance criteria:

Unit test
Integration test
Documentation
I have read the test guidelines

How to test changes / Special notes to the reviewer:

girishramnani · 2020-10-30T06:04:41Z

@dgolovin This might change the odo list -o json output, please confirm.

kadel · 2020-11-02T13:56:12Z

/approve

openshift-ci-robot · 2020-11-02T13:56:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kadel

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:

~~OWNERS~~ [kadel]

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

prietyc123 · 2020-11-04T12:32:10Z

@girishramnani unit test TestList for List function are failing due to component field mismatch. Could you please fix those.

girishramnani · 2020-11-04T19:24:00Z

unit test should be resolved now

codecov · 2020-11-05T13:23:15Z

Codecov Report

Merging #4177 (06171fa) into master (0dea26a) will decrease coverage by 0.21%.
The diff coverage is 64.51%.

@@            Coverage Diff             @@
##           master    #4177      +/-   ##
==========================================
- Coverage   42.56%   42.35%   -0.22%     
==========================================
  Files         156      154       -2     
  Lines       13266    13197      -69     
==========================================
- Hits         5647     5589      -58     
+ Misses       7017     6999      -18     
- Partials      602      609       +7

Impacted Files	Coverage Δ
pkg/component/pushed_component.go	`57.69% <ø> (ø)`
pkg/odo/util/completion/completionhandlers.go	`35.20% <ø> (ø)`
pkg/url/url.go	`73.41% <ø> (-0.23%)`	⬇️
pkg/component/component.go	`23.33% <64.51%> (+0.88%)`	⬆️
pkg/kclient/volumes.go	`89.36% <0.00%> (-8.32%)`	⬇️
...g/devfile/adapters/kubernetes/component/adapter.go	`31.11% <0.00%> (-6.43%)`	⬇️
pkg/devfile/adapters/kubernetes/utils/utils.go	`80.66% <0.00%> (-0.36%)`	⬇️
pkg/catalog/catalog.go	`50.55% <0.00%> (ø)`
pkg/kclient/supervisord.go	`0.00% <0.00%> (ø)`
pkg/testingutil/devfile.go	`0.00% <0.00%> (ø)`
... and 18 more

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 809462a...7d9143a. Read the comment docs.

dharmit · 2020-11-11T07:10:22Z

pkg/component/component.go

@@ -856,8 +856,33 @@ func GetComponentNames(client *occlient.Client, applicationName string) ([]strin
 	return names, nil
 }

-// List lists components in active application
-func List(client *occlient.Client, applicationName string, localConfigInfo *config.LocalConfigInfo) (ComponentList, error) {
+func ListDevfileComponents(client *occlient.Client, selector string) (ComponentList, error) {


Please add comments.

+1 especially description for selector.
It looks a little bit weird to me as we don't use selectors for odo components. Why not keep applicationName are we ever going to use something else then application name selector?
ListS2IComponents has applicationName argument, it would be nice to match that.

yes @kadel - we use the project selector when we want to have components from all application, but in case of s2i components - we list all applications and then list per component application
https://github.com/openshift/odo/blob/e5798c42c9ce191425e3ed18e52db985e1e2f920/pkg%2Fodo%2Fcli%2Fcomponent%2Flist.go#L254-L261

Now the S2I approach is not optimised as it Xn calls to the where n is the number of applications and X is the calls done per application.

dharmit · 2020-11-11T07:52:03Z

pkg/component/component.go

+		components = append(components, component)
+	}
+
+	compoList := GetMachineReadableFormatForList(components)


+1 for returning the machine readable output here.

dharmit · 2020-11-11T07:56:49Z

pkg/component/component.go

+// List lists all components in active application
+func List(client *occlient.Client, applicationName string, localConfigInfo *config.LocalConfigInfo) (ComponentList, error) {
+	var applicationSelector string
+	if applicationName != "" {


If the applicationName field will be "" or nil for some reason, what would happen to the calls to ListDevfileComponents and ListS2Icomponents below?

Is there going to be a situation where this field could be empty? Isn't that something that CLI level should prevent from happening?

I am not exactly sure if CLI prevents it but as List actually checks the local component as well - chances are that doesn't require application name

pkg/component/component.go

pkg/odo/cli/component/list.go

dharmit · 2020-11-11T08:07:54Z

pkg/component/types.go

-	Type       string `json:"type,omitempty"`
-	SourceType string `json:"sourceType,omitempty"`
+	S2IComponents     []Component `json:"s2iComponents"`
+	DevfileComponents []Component `json:"devfileComponents"`


Thinking out loud. Can we have Items and make S2IComponents & DevfileComponents a part of it? I'm asking this because it'll help us have uniformity with the way k8s returns a list of items. This should also cascade to the way we return list of services, AFAIK.

I am thinking as part of refactor we might consolidate these two list with a new label called IsLegacy once devfile matures further.

So you mean that we can have just one list called Items and then in the individual items, there could be a bool label IsLegacy which indicates that it's an s2i component, right? Sounds good to me. Or we could label it IsS2i. But that's a separate discussion for when we refactor it.

pkg/component/component.go

dharmit · 2020-11-12T15:30:19Z

@girishramnani will this PR also fix #4023? Not asking that it should, just wondering if it would.

girishramnani · 2020-11-17T08:08:34Z

@girishramnani will this PR also fix #4023? Not asking that it should, just wondering if it would.

actually that one has a bigger problem which we might not be able to solve right now

dharmit · 2020-11-19T08:50:53Z

@girishramnani Multiple test cases failing in unit test

dharmit

/lgtm

openshift-bot · 2020-11-25T06:16:00Z

/retest

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

openshift-bot · 2020-11-25T06:41:58Z

/retest

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

openshift-ci-robot added kind/bug Categorizes issue or PR as related to a bug. kind/cleanup labels Oct 30, 2020

openshift-ci-robot requested review from adisky and mohammedzee1000 October 30, 2020 06:01

girishramnani changed the title ~~remove duplicate list and major refactor~~ remove duplicate list and refactor Oct 30, 2020

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label Nov 2, 2020

girishramnani force-pushed the duplicate_list branch from 78623e5 to bf95aaf Compare November 4, 2020 19:07

dharmit suggested changes Nov 11, 2020

View reviewed changes

openshift-ci-robot assigned dharmit Nov 11, 2020

kadel reviewed Nov 11, 2020

View reviewed changes

pkg/component/component.go Outdated Show resolved Hide resolved

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Nov 15, 2020

girishramnani force-pushed the duplicate_list branch from 06171fa to 047056f Compare November 19, 2020 07:38

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Nov 19, 2020

girishramnani added 8 commits November 24, 2020 21:19

remove duplicate list and refactor

7514f39

resolve failing validation

b42e526

reordered the listing

35bb41d

add source type in devfile components

e7fb079

add intg test for odo list duplicate

0e73a1f

removed FIt

6f7f7ef

resolve comments

b0f0b03

uniformity

9ec4963

girishramnani force-pushed the duplicate_list branch from c6cf27c to 9ec4963 Compare November 24, 2020 15:49

resolve failing tests and uniform labels in tests

7d9143a

dharmit approved these changes Nov 25, 2020

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Nov 25, 2020

openshift-merge-robot merged commit 82c0570 into redhat-developer:master Nov 25, 2020

rm3l added the area/refactoring Issues or PRs related to code refactoring label Jun 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove duplicate list and refactor #4177

remove duplicate list and refactor #4177

girishramnani commented Oct 30, 2020 •

edited

girishramnani commented Oct 30, 2020

kadel commented Nov 2, 2020

openshift-ci-robot commented Nov 2, 2020

prietyc123 commented Nov 4, 2020

girishramnani commented Nov 4, 2020

codecov bot commented Nov 5, 2020 •

edited

dharmit Nov 11, 2020

kadel Nov 11, 2020 •

edited

girishramnani Nov 19, 2020

girishramnani Nov 19, 2020

dharmit Nov 11, 2020

dharmit Nov 11, 2020

girishramnani Nov 19, 2020

dharmit Nov 11, 2020

girishramnani Nov 19, 2020

dharmit Nov 19, 2020

girishramnani Nov 24, 2020

dharmit commented Nov 12, 2020

girishramnani commented Nov 17, 2020

dharmit commented Nov 19, 2020

dharmit left a comment

openshift-bot commented Nov 25, 2020

openshift-bot commented Nov 25, 2020

remove duplicate list and refactor #4177

remove duplicate list and refactor #4177

Conversation

girishramnani commented Oct 30, 2020 • edited

girishramnani commented Oct 30, 2020

kadel commented Nov 2, 2020

openshift-ci-robot commented Nov 2, 2020

prietyc123 commented Nov 4, 2020

girishramnani commented Nov 4, 2020

codecov bot commented Nov 5, 2020 • edited

Codecov Report

Choose a reason for hiding this comment

kadel Nov 11, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dharmit commented Nov 12, 2020

girishramnani commented Nov 17, 2020

dharmit commented Nov 19, 2020

dharmit left a comment

Choose a reason for hiding this comment

openshift-bot commented Nov 25, 2020

openshift-bot commented Nov 25, 2020

girishramnani commented Oct 30, 2020 •

edited

codecov bot commented Nov 5, 2020 •

edited

kadel Nov 11, 2020 •

edited