Include imagestreams in current namespace for catalog listing #618

anmolbabu · 2018-08-04T13:07:06Z

This PR considers imagestream in current namespace along with
imagestreams in openshift namespace for:

catalog listing i.e, odo catalog list is now expected to show items in local
namespace along with those in openshift namespace
component create i.e odo create <comp_name> --git <git_url> is expected
to work even if comp_name is present in current namespace but not in openshift
namespace

fixes #566
Signed-off-by: anmolbabu anmolbudugutta@gmail.com

anmolbabu · 2018-08-04T13:10:49Z

@surajnarwade I tried grepping for any other usages of occlient.OpenShiftNameSpace and also its equivalent "openshift" but didn't find any other the one in this PR, can you please help me find which other areas of code need to undergo changes as per last paragraph of comment in #566 (comment)

surajnarwade · 2018-08-06T09:19:34Z

How to test ?

Use this link to create imagestream in current namespace, https://paste.fedoraproject.org/paste/JBld1qYlLozPWqYcFyC32g
oc create -f filename
oc get imagestream

NAME                      DOCKER REPO                                         TAGS                   UPDATED
nodejs-currentnamespace   172.30.1.1:5000/myproject/nodejs-currentnamespace   0.10,4,6 + 2 more...

odo catalog list

$ odo catalog list
NAME                        TAGS
dotnet                      2.0,latest
httpd                       2.4,latest
nginx                       1.10,1.12,1.8,latest
nodejs                      0.10,4,6,8,latest
perl                        5.16,5.20,5.24,latest
php                         5.5,5.6,7.0,7.1,latest
python                      2.7,3.3,3.4,3.5,3.6,latest
ruby                        2.0,2.2,2.3,2.4,latest
wildfly                     10.0,10.1,8.1,9.0,latest
nodejs-currentnamespace     0.10,4,6,8,latest

surajnarwade · 2018-08-06T09:20:18Z

pkg/catalog/catalog.go

 func getDefaultBuilderImages(client *occlient.Client) ([]CatalogImage, error) {
 	imageStreams, err := client.GetImageStreams(occlient.OpenShiftNameSpace)
 	if err != nil {
 		return nil, errors.Wrap(err, "unable to get Image Streams")
 	}

+	imageStreams2, err := client.GetImageStreams(client.GetCurrentProjectName())
+	if err != nil {
+		return nil, errors.Wrap(err, "unable to get Image Streams")


unable to get Image Streams from current project

anmolbabu · 2018-08-06T13:30:29Z

Verified as in :

[redhat@localhost redhat-developer]$ oc project
Using project "myproject" on server "https://127.0.0.1:8443".

[redhat@localhost redhat-developer]$ oc get imagestreams -n myproject
NAME DOCKER REPO TAGS UPDATED
nodejs-currentnamespace 172.30.1.1:5000/myproject/nodejs-currentnamespace 8,latest,0.10 + 2 more...

[redhat@localhost redhat-developer]$ odo catalog list
NAME TAGS
dotnet 2.0,latest
httpd 2.4,latest
nginx 1.10,1.12,1.8,latest
nodejs 0.10,4,6,8,latest
perl 5.16,5.20,5.24,latest
php 5.5,5.6,7.0,7.1,latest
python 2.7,3.3,3.4,3.5,3.6,latest
ruby 2.0,2.2,2.3,2.4,latest
wildfly 10.0,10.1,8.1,9.0,latest
nodejs-currentnamespace 0.10,4,6,8,latest

anmolbabu · 2018-08-06T15:18:27Z

@surajnarwade can you please revisit the PR I have incorporated your comments..
Please also have a look at #618 (comment)

kadel

odo create needs also work with images from the user's namespace.
Sorry if it wasn't obvious from the original issue :-(
The main point is that user has to be able to create new component using builder image that is in his namespace.

kadel · 2018-08-07T09:20:55Z

pkg/catalog/catalog.go

 	}

+	imageStreams2, err := client.GetImageStreams(client.GetCurrentProjectName())


it might be better to have a more descriptive name than imageStreams2

anmolbabu · 2018-08-07T10:13:27Z

pkg/catalog/catalog_test.go

+		name    string
+		args    args
+		wantErr bool
+		wantOp  []CatalogImage // Expect 1 duplicate because of 2 calls(1 for current namespace and another for openshift namespace) to client.GetImageStreams per call to list


@kadel Is there a better way of doing this test
i.e, https://github.com/redhat-developer/odo/pull/618/files#diff-1dfbb1796e8d3ca31e26fe9d003bb488R188 for particular parameters instead of whole list?

I think that this is ok, but the duplicate stuff is a bit weird

The duplicate is because of 2 calls to the same function(although different params) https://github.com/redhat-developer/odo/pull/618/files#diff-6025e9b620e9321c03c11bf29e5c4dbdR401 and https://github.com/redhat-developer/odo/pull/618/files#diff-6025e9b620e9321c03c11bf29e5c4dbdR391 and the function to mock the call isn't per parameter(as in return x when function called with y as parameter else return z) and the mocking is to api function not its wrapper that is immediately used in our func...

syamgk · 2018-08-07T13:56:35Z

pkg/catalog/catalog.go

 	}

+	imageStreams2, err := client.GetImageStreams(client.GetCurrentProjectName())
+	if err != nil {
+		return nil, errors.Wrapf(err, "unable to get Image Streams from %s namespace", imageStreams2)


In this return statement it should be client.GetCurrentProjectName() instead of imageStreams2

kadel · 2018-08-07T14:58:45Z

pkg/catalog/catalog_test.go

+		tags               []string
+		currentNSImageName string
+		currentNamespace   string
+		currentNSImageTags []string


what is this variable for?

There are 2 calls to the same function(although different params) https://github.com/redhat-developer/odo/pull/618/files#diff-6025e9b620e9321c03c11bf29e5c4dbdR401 and https://github.com/redhat-developer/odo/pull/618/files#diff-6025e9b620e9321c03c11bf29e5c4dbdR391 and the function to mock the call isn't per parameter(as in return x when function called with y as parameter else return z) and the mocking is to api function not its wrapper that is immediately used in our func...

Hence, inorder for the mocking function to be able to return values from both namespaces this was the only way possible because the mocking is not per parameter

kadel · 2018-08-07T15:00:50Z

pkg/catalog/catalog_test.go

@@ -140,7 +186,7 @@ func TestList(t *testing.T) {
 			// Fake the client with the appropriate arguments
 			client, fakeClientSet := occlient.FakeNew()
 			fakeClientSet.ImageClientset.PrependReactor("list", "imagestreams", func(action ktesting.Action) (bool, runtime.Object, error) {
-				return true, testingutil.FakeImageStreams(tt.args.name, tt.args.namespace, tt.args.tags), nil
+				return true, testingutil.FakeImageStreams(tt.args.name, tt.args.namespace, tt.args.tags, tt.args.currentNSImageName, tt.args.currentNamespace, tt.args.currentNSImageTags), nil


why not just call FakeImageStreams twice? Once to add images to "openshift" namespace, second time to add them to the other namespace. You don't have to modify FakeImageStreams function at all.

or add it two prependReactor calls

will try this
but how would this work? the prependReactor doesn't take params as arg right to expect it to return 2 different NS values for 2 different calls based on params passed?

@kadel Hey but it worked

kadel · 2018-08-07T15:07:01Z

pkg/catalog/catalog.go

@@ -87,13 +87,19 @@ func VersionExists(client *occlient.Client, componentType string, componentVersi
 }

 // getDefaultBuilderImages returns the default builder images available in the
-// openshift namespace
+// openshift namespace and current namespace
 func getDefaultBuilderImages(client *occlient.Client) ([]CatalogImage, error) {


This image should also return in what namespace the image is. as there might be two images with the same name in both openshift and current namespace. We also have to figure out what we will do if there is a collision.

Ack

The o/p now looks like this: https://gist.github.com/anmolbabu/028a4caf25c009e3c63c82721f8146f3

surajnarwade · 2018-08-09T05:27:34Z

$ odo catalog list
NAME        TAGS
dotnet      2.0,latest
httpd       2.4,latest
nginx       1.10,1.12,1.8,latest
nodejs      0.10,4,6,8,latest
perl        5.16,5.20,5.24,latest
php         5.5,5.6,7.0,7.1,latest
python      2.7,3.3,3.4,3.5,3.6,latest
ruby        2.0,2.2,2.3,2.4,2.5,latest
wildfly     10.0,10.1,11.0,12.0,8.1,9.0,latest
mynodjs      0.10,4,6,8,latest
$ odo create mynodejs
Invalid component type: mynodejs
Run 'odo catalog list' to see a list of supported components

It can list the new imagestream, but can not use it

kadel · 2018-08-09T10:35:54Z

How are we going to solve the duplicity issue? What if the same image/ImageStream is in openshift project and in the current project?

anmolbabu · 2018-08-13T06:03:34Z

Still facing the issue:

Error resolving ImageStreamTag python:latest in namespace openshift: imagestreams.image.openshift.io "python" not found

Probably an additional param/config needs to be passed to build intialise request

surajnarwade · 2018-08-13T08:52:51Z

pkg/occlient/occlient.go

+			return nil, errors.Wrapf(err, "no match found for : %s in namespace %s. error :%v", imageName, currentProjectName, e)
+		}
+		// Ignore the error if there was no error previously as the desired imagestream might be only in openshift namespace
+		err = nil


you dont need this

kadel · 2018-08-13T09:42:23Z

pkg/catalog/catalog.go

-	imageStreams, err := client.GetImageStreams(occlient.OpenShiftNameSpace)
+// openshift namespace and current namespace
+// Return  map[imageName: string]map[namespace:string][]tags:string
+func getDefaultBuilderImages(client *occlient.Client) (map[string]map[string][]string, error) {


uff this is pretty complicated return type. Wouldn't it be easier to create some nice struct for this?

Iteration of struct array becomes tedious for any operations like search and delete.
But in a map the access is O(1) for most cases although at the cost of ease of code maintenance.
But it might now boil down to likely max size of list of imagestreams in current NS and openshift NS which looking at default size in my minishift deployment seems very small to bother about time complexities..

So, I am changing https://github.com/redhat-developer/odo/pull/618/files#diff-4373e1ac6258d2d589cb87e96b1fc994R13 to

type CatalogImage struct { Name string NamespacedTags map[Namespace: string]Tags: []string }

surajnarwade · 2018-08-13T09:46:22Z

What we need to do for this PR ?

GetExposedPorts function should return namespace along with ports as well. This function should also decide which namespace to chose in case of collision (current namespace and openshift namespace consist of same image). Ideally, it should take current namespace, because Dev might have some intention behind this to create one.
In BootstrapSupervisoredS2I, use the namespace given by GetExposedPorts function here

kadel · 2018-08-13T09:56:50Z

In BootstrapSupervisoredS2I, use the namespace given by GetExposedPorts function here

the same for NewAppS2I(name string, builderImage string, gitUrl string, labels map[string]string, annotations map[string]string, inputPorts []string)

surajnarwade · 2018-08-13T10:06:43Z

@anmolbabu , if there's an issue with creating imagestream, follow this: http://suraj.pro/post/access-minishift-registry/

jorgemoralespou · 2018-08-13T10:43:41Z

@kadel As how openshift works currently, and I think it might make sense, when there's imagestreams that are duplicated, in openshift namespace and current namespace, we should qualify the image or else, the local namespace should take precendence. Probably would also be good to show in the catalog listing as well:

$ odo catalog list
NAME        TAGS
openshift/nodejs      0.10,4,6,8,latest
nodejs      0.10,4,6,8,latest

Or add a visual indication to the printed list:

$ odo catalog list
NAME        TAGS                     CATALOG
nodejs        0.10,4,6,8,latest    openshift
nodejs (*)   0.10,4,6,8,latest    my-project 

NOTE: There might be catalog items in multiple catalogs. Those marked with (*) will take precedence. In case you want to use an item from a different catalog, please fully qualify it:
e.g. 

$ odo create openshift/python ....

or

$ odo create python ..... --catalog=openshift

Then, it would be obvious which one is used. I used the term catalog here in my explanation although it might not be the best term.

This PR adds `*` to the catalog that will be considered on priority for component creation in cases when a imagestream of same name exists in both openshift and the current namespaces. fixes redhat-developer#566 Signed-off-by: anmolbabu <anmolbudugutta@gmail.com>

Extract the component type part from passed fully qualified or un-qualified default component type and use it as default chosen component name fixes redhat-developer#566 Signed-off-by: anmolbabu <anmolbudugutta@gmail.com>

fixes redhat-developer#566 Signed-off-by: anmolbabu <anmolbudugutta@gmail.com>

Signed-off-by: anmolbabu <anmolbudugutta@gmail.com>

cdrage · 2018-09-04T16:17:14Z

Thanks for being patient with the added tests! This LGTM! Thanks so much @anmolbabu

anmolbabu requested a review from surajnarwade August 4, 2018 13:07

anmolbabu added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Aug 4, 2018

surajnarwade suggested changes Aug 6, 2018

View reviewed changes

anmolbabu requested review from kadel, syamgk, cdrage and mik-dass August 6, 2018 13:20

anmolbabu removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Aug 6, 2018

kadel previously requested changes Aug 7, 2018

View reviewed changes

anmolbabu commented Aug 7, 2018

View reviewed changes

syamgk suggested changes Aug 7, 2018

View reviewed changes

kadel reviewed Aug 7, 2018

View reviewed changes

anmolbabu force-pushed the issue#566 branch from 166691a to a425179 Compare August 10, 2018 07:27

anmolbabu added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. Required by Prow. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. labels Aug 10, 2018

surajnarwade reviewed Aug 13, 2018

View reviewed changes

kadel reviewed Aug 13, 2018

View reviewed changes

anmolbabu added 18 commits September 4, 2018 20:22

Fix component name creation from component type

8f2200b

Extract the component type part from passed fully qualified or un-qualified default component type and use it as default chosen component name fixes redhat-developer#566 Signed-off-by: anmolbabu <anmolbudugutta@gmail.com>

Incoporate @tkral comments

087caa0

fixes redhat-developer#566 Signed-off-by: anmolbabu <anmolbudugutta@gmail.com>

Incorporate @ashetty1 comments

54e8331

fixes redhat-developer#566 Signed-off-by: anmolbabu <anmolbudugutta@gmail.com>

Fix validate errors

5e74391

Signed-off-by: anmolbabu <anmolbudugutta@gmail.com>

Add e2e test

2845163

Signed-off-by: anmolbabu <anmolbudugutta@gmail.com>

Rebase

648e4ab

Signed-off-by: anmolbabu <anmolbudugutta@gmail.com>

Incoporated @tkral comments

6e3b5c8

Signed-off-by: anmolbabu <anmolbudugutta@gmail.com>

Incoporate @ashetty1 comments

8aeb337

Signed-off-by: anmolbabu <anmolbudugutta@gmail.com>

Incorporate @cdrage comments

0fd1a7b

Signed-off-by: anmolbabu <anmolbudugutta@gmail.com>

Incoporate @snarwade comments

152aa0a

Signed-off-by: anmolbabu <anmolbudugutta@gmail.com>

Fix e2e test after rebase

b9ea7ff

Signed-off-by: anmolbabu <anmolbudugutta@gmail.com>

Incoporate @cdrage comments

07a88cf

Signed-off-by: anmolbabu <anmolbudugutta@gmail.com>

Incoporate @ashetty1 comments

c78e4e4

Signed-off-by: anmolbabu <anmolbudugutta@gmail.com>

Incorporate @cdrage comments

6895076

Signed-off-by: anmolbabu <anmolbudugutta@gmail.com>

Incoporate @syamgk comments

45bfde5

Signed-off-by: anmolbabu <anmolbudugutta@gmail.com>

Incoporate @cdrage comments

e92a3ef

Signed-off-by: anmolbabu <anmolbudugutta@gmail.com>

Incoporate @cdrage comments

30e3d25

Signed-off-by: anmolbabu <anmolbudugutta@gmail.com>

anmolbabu force-pushed the issue#566 branch from 6eb3b3a to 30e3d25 Compare September 4, 2018 15:19

cdrage approved these changes Sep 4, 2018

View reviewed changes

cdrage merged commit f37a36a into redhat-developer:master Sep 4, 2018

anmolbabu deleted the issue#566 branch September 4, 2018 18:16

snyk-bot mentioned this pull request Nov 29, 2021

[Snyk] Security upgrade @svgr/webpack from 5.5.0 to 6.0.0 adamkeinan/odo#15

Open

This was referenced Sep 30, 2022

[Snyk] Fix for 16 vulnerabilities adamkeinan/odo#50

Open

[Snyk] Fix for 16 vulnerabilities adamkeinan/odo#51

Open

[Snyk] Fix for 16 vulnerabilities adamkeinan/odo#52

Open

adamkeinan mentioned this pull request Oct 7, 2022

[Snyk] Fix for 16 vulnerabilities adamkeinan/odo#53

Open

adamkeinan mentioned this pull request May 22, 2023

[Snyk] Fix for 1 vulnerabilities adamkeinan/odo#77

Open

adamkeinan mentioned this pull request Dec 19, 2023

[Snyk] Security upgrade @svgr/webpack from 5.5.0 to 6.0.0 adamkeinan/odo#97

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include imagestreams in current namespace for catalog listing #618

Include imagestreams in current namespace for catalog listing #618

anmolbabu commented Aug 4, 2018 •

edited

anmolbabu commented Aug 4, 2018

surajnarwade commented Aug 6, 2018

surajnarwade Aug 6, 2018

anmolbabu Aug 6, 2018

anmolbabu commented Aug 6, 2018

anmolbabu commented Aug 6, 2018

kadel left a comment

kadel Aug 7, 2018

anmolbabu Aug 7, 2018

kadel Aug 7, 2018

anmolbabu Aug 14, 2018

syamgk Aug 7, 2018 •

edited

anmolbabu Aug 10, 2018

kadel Aug 7, 2018

anmolbabu Aug 14, 2018

kadel Aug 7, 2018

kadel Aug 7, 2018

anmolbabu Aug 14, 2018

anmolbabu Aug 16, 2018

kadel Aug 7, 2018

anmolbabu Aug 10, 2018

surajnarwade commented Aug 9, 2018

kadel commented Aug 9, 2018

anmolbabu commented Aug 13, 2018

surajnarwade Aug 13, 2018

anmolbabu Aug 18, 2018

kadel Aug 13, 2018

anmolbabu Aug 14, 2018

surajnarwade commented Aug 13, 2018 •

edited

kadel commented Aug 13, 2018

surajnarwade commented Aug 13, 2018

jorgemoralespou commented Aug 13, 2018

cdrage commented Sep 4, 2018

		}

		imageStreams2, err := client.GetImageStreams(client.GetCurrentProjectName())

Include imagestreams in current namespace for catalog listing #618

Include imagestreams in current namespace for catalog listing #618

Conversation

anmolbabu commented Aug 4, 2018 • edited

anmolbabu commented Aug 4, 2018

surajnarwade commented Aug 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anmolbabu commented Aug 6, 2018

anmolbabu commented Aug 6, 2018

kadel left a 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

syamgk Aug 7, 2018 • 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

surajnarwade commented Aug 9, 2018

kadel commented Aug 9, 2018

anmolbabu commented Aug 13, 2018

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

surajnarwade commented Aug 13, 2018 • edited

kadel commented Aug 13, 2018

surajnarwade commented Aug 13, 2018

jorgemoralespou commented Aug 13, 2018

cdrage commented Sep 4, 2018

anmolbabu commented Aug 4, 2018 •

edited

syamgk Aug 7, 2018 •

edited

surajnarwade commented Aug 13, 2018 •

edited