Dropping support for service catalog based services #4906

mohammedzee1000 · 2021-07-12T13:10:34Z

Signed-off-by: Mohammed Zeeshan Ahmed mohammed.zee1000@gmail.com

What type of PR is this?
/kind code-refactoring
/kind cleanup

What does this PR do / why we need it:
We are dropping support for service catalog based services

Which issue(s) this PR fixes:

Fixes #4890
Fixes #5004

PR acceptance criteria:

How to test changes / Special notes to the reviewer:

odo catalog list services should not list service catalog even if it is available in cluster

> ./odo catalog list services
Services available through Operators
NAME                                CRDs
redis-operator.v0.6.0               RedisCluster, Redis
service-binding-operator.v0.8.0     ServiceBinding

odo catalog describe service x/y should describe operator backed service and fail otherwise

> odo catalog describe service blah
 ✗  unable to search classes by name (the server could not find the requested resource (get clusterserviceclasses.servicecatalog.k8s.io))
> odo catalog describe service redis-operator.v0.6.0/Redis
Kind: Redis
Version: v1beta1
Description: Redis
Parameters:

 PATH  DISPLAYNAME  DESCRIPTION

odo service create should successfully create operator backed services and attempts to create service catalog should fail

> odo service create blah
 ✗  invalid service name, use the format <operator-type>/<crd-name>
> odo service create redis-operator.v0.6.0/Redis
Successfully added service to the configuration; do 'odo push' to create service on the cluster

odo service list should only list operator backed services
odo service delete should successfully delete operator backed services

netlify · 2021-07-14T07:22:08Z

✔️ Deploy Preview for odo-docusaurus-preview ready!

🔨 Explore the source changes: 9571113

🔍 Inspect the deploy log: https://app.netlify.com/sites/odo-docusaurus-preview/deploys/6119f3cb9650f10008db0189

😎 Browse the preview: https://deploy-preview-4906--odo-docusaurus-preview.netlify.app

mohammedzee1000 · 2021-07-14T12:19:46Z

prow error
/retest

mohammedzee1000 · 2021-07-16T09:17:26Z

/refresh

mohammedzee1000 · 2021-07-16T13:05:30Z

timeout
/test psi-unit-test-windows

mohammedzee1000 · 2021-07-19T06:51:32Z

timeouts
/retest

mohammedzee1000 · 2021-07-19T09:13:48Z

timed out
/retest

mohammedzee1000 · 2021-07-19T12:36:36Z

ci error
/retest

mohammedzee1000 · 2021-07-20T10:37:59Z

macos machine issue
/test psi-unit-test-mac

mohammedzee1000 · 2021-07-22T08:58:38Z

/test v4.7-integration-e2e

mohammedzee1000 · 2021-07-22T13:03:55Z

/retest

mohammedzee1000 · 2021-07-22T15:31:20Z

/retest

mohammedzee1000 · 2021-07-23T05:43:40Z

/retest

dharmit · 2021-07-22T11:42:57Z

pkg/odo/cli/catalog/describe/service.go

-	service svc.ServiceClass
-	plans   []svc.ServicePlan
+	// service svc.ServiceClass
+	// plans   []svc.ServicePlan


These fields are all related to Service Catalog. Remove them. We don't need them.

dharmit · 2021-07-22T11:43:56Z

pkg/odo/cli/catalog/describe/service.go

-		o.backend = NewServiceCatalogBackend()
+	_, _, err = service.IsOperatorServiceNameValid(args[0])
+	if err != nil {
+		return fmt.Errorf("invalid operator name and service catalog not supported %w", err)


We don't need to mention in CLI output that Service Catalog is not supported anymore. We should put it on our project documentation on the website and the changelog.

dharmit · 2021-07-22T11:46:18Z

pkg/odo/cli/catalog/search/service.go

-		// Kubernetes or OpenShift 4.x cluster. It doesn't occur when there are
-		// no operators installed.


I don't think we should remove these comments.

I can still see them

I'm seeing below:

// Error only occurs when OperatorHub is not installed/enabled on the // Kubernetes or OpenShift 4.x cluster. // Neither OperatorHub nor Service Catalog is enabled on the cluster

IMO, it should be:

// Error only occurs when OperatorHub is not installed/enabled on the // Kubernetes or OpenShift 4.x cluster. It doesn't occur when there are // no operators installed. // OperatorHub is not enabled on the cluster

Updated ptal

dharmit · 2021-07-22T11:47:41Z

pkg/odo/cli/component/common_link.go

 	cmpExists, err := component.Exists(o.Client, suppliedName, o.Application)
 	if err != nil {
 		return fmt.Errorf("Unable to determine if component exists:\n%v", err)
 	}

-	if !cmpExists && !svcExists {
+	if !cmpExists {
 		return fmt.Errorf("Neither a service nor a component named %s could be located. Please create one of the two before attempting to use 'odo %s'", suppliedName, o.operationName)


I know it's not modified as a part of this PR but errors shouldn't start with capital letter. Can you please change it?

Suggested change

return fmt.Errorf("Neither a service nor a component named %s could be located. Please create one of the two before attempting to use 'odo %s'", suppliedName, o.operationName)

return fmt.Errorf("neither a service nor a component named %s could be located. Please create one of the two before attempting to use 'odo %s'", suppliedName, o.operationName)

dharmit · 2021-07-23T05:55:45Z

pkg/odo/cli/service/list.go

-		if o.Context.Project == "" || o.Context.Application == "" {
-			return odoutil.ThrowContextError()
-		}
+		return fmt.Errorf("operator backed services not installed and service catalog is no longer supported")


Same suggestion. We don't need to mention Service Catalog on CLI any more.

dharmit · 2021-07-23T05:56:24Z

pkg/odo/cli/service/service_test.go

-	}
-	return true
-}
+//func equal(s1, s2 []string) bool {


Why are we commenting? We can just remove it, no?

removed file as it is last thing remaining

dharmit · 2021-07-23T05:57:17Z

pkg/odo/cli/service/ui/ui.go

 	"k8s.io/klog"

+	scv1beta1 "github.com/kubernetes-sigs/service-catalog/pkg/apis/servicecatalog/v1beta1"


This import corresponds to Service Catalog. Is something in this file still doing something with it? Can we remove it?

dharmit · 2021-07-23T06:01:15Z

tests/integration/operatorhub/cmd_service_test.go

+		When("user runs odo service commands with help flag", func() {
+			It("should display the help for odo service -h", func() {
+				appHelp := helper.Cmd("odo", "service", "-h").ShouldPass().Out()
+				Expect(appHelp).To(ContainSubstring("Perform service catalog operations"))


IMO, we sholdn't be showing "service catalog" on CLI. I couldn't find another occurrence of this string being checked with a Ctrl+F. We should remove it, IMO.

Co-authored-by: Parthvi Vala <pvala@redhat.com>

Signed-off-by: Mohammed Zeeshan Ahmed <mohammed.zee1000@gmail.com>

mohammedzee1000 · 2021-08-16T05:13:53Z

reset and rebased, now il ensure some of the docs changes did not disappear

Signed-off-by: Mohammed Zeeshan Ahmed <mohammed.zee1000@gmail.com>

openshift-ci · 2021-08-16T12:14:39Z

@mohammedzee1000: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Rerun command
ci/prow/v4.7-integration-e2e	`5f5d898`	link	`/test v4.7-integration-e2e`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

mohammedzee1000 · 2021-08-16T12:26:36Z

project json format error
/test v4.8-integration-e2e

valaparthvi · 2021-08-17T08:12:15Z

I think people are confusing odo catalog service v/s openshift service catalog v/s Service Catalog Backend. I was confused at first too but the reason why service catalog is used here is that openshift service catalog was the only way to create services in the days this code was written.
In later stages, we got operators and potentially helm in the future. But the name service catalog has stuck (#4906 (comment))

Then perhaps we should consider renaming the interface to something like Service Backend or something less ambiguous. But again, out of scope for this PR.

feloy

Some little comments, LGTM otherwise

pkg/odo/cli/catalog/describe/service.go

feloy · 2021-08-17T09:10:54Z

pkg/odo/cli/catalog/describe/service.go

@@ -59,7 +47,7 @@ func (o *DescribeServiceOptions) Complete(name string, cmd *cobra.Command, args
 	if _, _, err := service.SplitServiceKindName(args[0]); err == nil {


You can as usual handle the error case first:

if _, _, err := service.SplitServiceKindName(args[0]); err != nil { return fmt.Errorf("no deployable operators found") } o.backend = NewOperatorBackend()

pkg/odo/cli/service/list.go

valaparthvi

I have a few concerns about the deleted unit tests and completion handlers, which still seem relevant, but they definitely need refactoring. Maybe we can comment the tests for now, and refactor them to work with services that we still support, i.e. Operator Backed Services.

Other than that, since the integration tests pass and no other irrelevant integration tests were deleted or modified, I am inclined to do a lgtm.

valaparthvi · 2021-08-17T08:27:24Z

pkg/odo/util/completion/completionhandlers_test.go

-	}
-}
-
-func TestLinkCompletionHandler(t *testing.T) {


I am not sure if we should completely remove these tests. I understand that they might need modification, but I don't think it's a good idea to completely remove them.

Recorded an issue

valaparthvi · 2021-08-17T08:35:19Z

pkg/odo/util/completion/completionhandlers_test.go

-	}
-}
-
-func TestUnlinkCompletionHandler(t *testing.T) {


Same comment as TestLinkCompletionHandlers.

Recorded in issue

valaparthvi · 2021-08-17T08:36:37Z

pkg/odo/util/completion/completionhandlers_test.go

-	}
-}
-
-func TestServiceClassCompletionHandler(t *testing.T) {


Which command does this handler test?

Recorded in issue

valaparthvi · 2021-08-17T08:42:09Z

pkg/service/service_test.go

@@ -1,9 +1,6 @@
 package service


Are you sure unit tests in this file are not relevant anymore, even with the operator backed services?

Recorded in issue before

valaparthvi · 2021-08-17T08:45:25Z

pkg/odo/cli/service/create_test.go

@@ -1,124 +0,0 @@
-package service


I don't think we should completely remove this unit test file, it is still relevant to operator backed services because we still have this command in odo. What it needs is refactoring.

Recorded in issue before

valaparthvi · 2021-08-17T08:53:25Z

pkg/odo/util/completion/completionhandlers.go

 	"github.com/openshift/odo/pkg/storage"
 	"github.com/openshift/odo/pkg/url"
 	"github.com/openshift/odo/pkg/util"
 	"github.com/posener/complete"
 	"github.com/spf13/cobra"
 )

-// ServiceCompletionHandler provides service name completion for the current project and application


Are you sure we want to remove this handler? Wouldn't it be helpful with the OBS?

Recorded in issue before

Co-authored-by: Philippe Martin <contact@elol.fr>

mohammedzee1000 · 2021-08-17T10:44:43Z

@valaparthvi opened issues as per discussion
#4996
#4997

Co-authored-by: Philippe Martin <contact@elol.fr>

sonarcloud · 2021-08-17T10:47:32Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

valaparthvi · 2021-08-17T10:58:51Z

@mohammedzee1000 I am curious, why have you not removed the serviceCatalog references from the kclient package? Are they still relevant or are you planning to take them up in part 2?
https://github.com/mohammedzee1000/odo/blob/remove_service_catalog/pkg/kclient/serviceCatalog.go
https://github.com/mohammedzee1000/odo/blob/remove_service_catalog/pkg/kclient/serviceCatalog_test.go

mohammedzee1000 · 2021-08-17T11:00:40Z

@mohammedzee1000 I am curious, why have you not removed the serviceCatalog references from the kclient package? Are they still relevant or are you planning to take them up in part 2?
https://github.com/mohammedzee1000/odo/blob/remove_service_catalog/pkg/kclient/serviceCatalog.go
https://github.com/mohammedzee1000/odo/blob/remove_service_catalog/pkg/kclient/serviceCatalog_test.go

Yes, there are still usages in link and unlink and both should be dropped simultanously

valaparthvi

No major changes have been made to the integration tests (except that of Service Catalog's) and they are all passing; all the remaining references will be removed in the next PR, hence I'm approving this PR.

openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. kind/code-refactoring labels Jul 12, 2021

openshift-ci bot requested review from dharmit and mik-dass July 12, 2021 13:10

mohammedzee1000 force-pushed the remove_service_catalog branch 2 times, most recently from fea4d1c to 0ce112d Compare July 14, 2021 06:31

mohammedzee1000 force-pushed the remove_service_catalog branch from 5b04584 to dea2b25 Compare July 16, 2021 06:26

mohammedzee1000 changed the title ~~WIP Dropping support for service catalog based services~~ Dropping support for service catalog based services Jul 16, 2021

openshift-ci bot 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 Jul 16, 2021

mohammedzee1000 force-pushed the remove_service_catalog branch from a3c592e to e77c423 Compare July 19, 2021 08:34

mohammedzee1000 force-pushed the remove_service_catalog branch from e77c423 to ef23dd5 Compare July 19, 2021 14:09

mohammedzee1000 force-pushed the remove_service_catalog branch from 167e549 to f075005 Compare July 22, 2021 06:09

mohammedzee1000 force-pushed the remove_service_catalog branch from f075005 to 596511a Compare July 22, 2021 13:04

dharmit mentioned this pull request Jul 22, 2021

Remove code related to s2i #4932

Closed

mohammedzee1000 force-pushed the remove_service_catalog branch from 596511a to 2602f7c Compare July 23, 2021 05:55

dharmit suggested changes Jul 23, 2021

View reviewed changes

openshift-ci bot assigned dharmit Jul 23, 2021

mohammedzee1000 requested a review from dharmit July 23, 2021 08:52

mohammedzee1000 and others added 5 commits August 16, 2021 10:42

Update pkg/odo/cli/service/create.go

4bd90d9

Co-authored-by: Parthvi Vala <pvala@redhat.com>

Fixing gofmt

8157746

Signed-off-by: Mohammed Zeeshan Ahmed <mohammed.zee1000@gmail.com>

Adding kube to cli docs

5265aff

Signed-off-by: Mohammed Zeeshan Ahmed <mohammed.zee1000@gmail.com>

Updating changelog as per comments by @dharmit

c2ad74f

Signed-off-by: Mohammed Zeeshan Ahmed <mohammed.zee1000@gmail.com>

Removing some unnessasary stuff

9571113

Signed-off-by: Mohammed Zeeshan Ahmed <mohammed.zee1000@gmail.com>

mohammedzee1000 force-pushed the remove_service_catalog branch from 52f9ab9 to 9571113 Compare August 16, 2021 05:12

mohammedzee1000 added 2 commits August 16, 2021 12:39

Updating docs based changes as per review

1c7587f

Signed-off-by: Mohammed Zeeshan Ahmed <mohammed.zee1000@gmail.com>

Fixing test

7cc5ec1

Signed-off-by: Mohammed Zeeshan Ahmed <mohammed.zee1000@gmail.com>

mohammedzee1000 requested review from valaparthvi and dharmit August 17, 2021 08:26

feloy reviewed Aug 17, 2021

View reviewed changes

valaparthvi suggested changes Aug 17, 2021

View reviewed changes

Update pkg/odo/cli/catalog/describe/service.go

8794667

Co-authored-by: Philippe Martin <contact@elol.fr>

mohammedzee1000 mentioned this pull request Aug 17, 2021

Add completion handlers for operator hub #4996

Closed

Update pkg/odo/cli/service/list.go

1578dc3

Co-authored-by: Philippe Martin <contact@elol.fr>

dharmit mentioned this pull request Aug 18, 2021

odo catalog describe service broken #5004

Closed

valaparthvi mentioned this pull request Aug 18, 2021

Add unit tests for operator hub based services #4997

Closed

valaparthvi approved these changes Aug 18, 2021

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Aug 18, 2021

openshift-ci bot merged commit 0b8b712 into redhat-developer:main Aug 18, 2021

mohitsuman mentioned this pull request Sep 14, 2021

Remove service catalog support related commands redhat-developer/vscode-openshift-tools#2208

Closed

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

		// Kubernetes or OpenShift 4.x cluster. It doesn't occur when there are
		// no operators installed.

	return fmt.Errorf("Neither a service nor a component named %s could be located. Please create one of the two before attempting to use 'odo %s'", suppliedName, o.operationName)
	return fmt.Errorf("neither a service nor a component named %s could be located. Please create one of the two before attempting to use 'odo %s'", suppliedName, o.operationName)

		"k8s.io/klog"

		scv1beta1 "github.com/kubernetes-sigs/service-catalog/pkg/apis/servicecatalog/v1beta1"

		@@ -59,7 +47,7 @@ func (o DescribeServiceOptions) Complete(name string, cmd cobra.Command, args
		if _, _, err := service.SplitServiceKindName(args[0]); err == nil {

Dropping support for service catalog based services #4906

Dropping support for service catalog based services #4906

Conversation

mohammedzee1000 commented Jul 12, 2021 • edited by dharmit Loading

netlify bot commented Jul 14, 2021 • edited Loading

mohammedzee1000 commented Jul 14, 2021

mohammedzee1000 commented Jul 16, 2021

mohammedzee1000 commented Jul 16, 2021

mohammedzee1000 commented Jul 19, 2021

mohammedzee1000 commented Jul 19, 2021

mohammedzee1000 commented Jul 19, 2021

mohammedzee1000 commented Jul 20, 2021

mohammedzee1000 commented Jul 22, 2021

mohammedzee1000 commented Jul 22, 2021

mohammedzee1000 commented Jul 22, 2021

mohammedzee1000 commented Jul 23, 2021

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

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

mohammedzee1000 commented Aug 16, 2021

openshift-ci bot commented Aug 16, 2021 • edited Loading

mohammedzee1000 commented Aug 16, 2021

valaparthvi commented Aug 17, 2021

feloy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valaparthvi 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

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

mohammedzee1000 commented Aug 17, 2021

sonarcloud bot commented Aug 17, 2021

valaparthvi commented Aug 17, 2021 • edited Loading

mohammedzee1000 commented Aug 17, 2021

valaparthvi left a comment

Choose a reason for hiding this comment

mohammedzee1000 commented Jul 12, 2021 •

edited by dharmit

Loading

netlify bot commented Jul 14, 2021 •

edited

Loading

openshift-ci bot commented Aug 16, 2021 •

edited

Loading

valaparthvi commented Aug 17, 2021 •

edited

Loading