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

Removing remaining service catalog references #5033

Merged

Conversation

mohammedzee1000
Copy link
Contributor

@mohammedzee1000 mohammedzee1000 commented Sep 1, 2021

What type of PR is this?
/kind cleanup

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

Which issue(s) this PR fixes:

Fixes #4978
Fixes #4011

PR acceptance criteria:

How to test changes / Special notes to the reviewer:
Operator hub service should not be broken
It should no longer be possible to anything with service catalog services

@openshift-ci 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/cleanup labels Sep 1, 2021
@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Sep 4, 2021
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Sep 8, 2021
@mohammedzee1000
Copy link
Contributor Author

mohammedzee1000 commented Sep 8, 2021

@mohammedzee1000 mohammedzee1000 changed the title WIP Removing remaining service catalog references Removing remaining service catalog references Sep 8, 2021
@openshift-ci 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 Sep 8, 2021
Copy link
Contributor

@feloy feloy left a comment

Choose a reason for hiding this comment

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

I can see some functions/types on service/service.go that are unused :

  • CreateOperatorService
  • DeleteServiceAndUnlinkComponents
  • GetGVKRFromCR
  • InstanceCreateParameterSchema

There are also these files:

  • pkg/kclient/serviceCatalog.go
  • pkg/kclient/serviceCatalog_test.go

metav1.TypeMeta `json:",inline"`
metav1.ListMeta `json:"metadata,omitempty"`
Items []Service `json:"items"`
// ServiceClass holds the information regarding a service catalog service class
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this comes from an old version. I cannot see how it is used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we want to remove them? they are operator related correct

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And yea il look at the files

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm talking about ServiceClass. It is not used anywhere on the code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

About the files I found there are functions that are not specific to service catalog. I will see what I can do

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry I was referring to the functions you listed, ServiceClass yea, it needs to go

// Kubernetes or OpenShift 4.x cluster. It doesn't occur when there are
// no operators installed.
noCsvs = true
return fmt.Errorf("unable to list services because Operator Hub is enabled in your cluster: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Operator Hub is not enabled

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you mean the error message?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@mohammedzee1000
Copy link
Contributor Author

retrying to be safe

/test psi-kubernetes-integration-e2e

@feloy
Copy link
Contributor

feloy commented Sep 10, 2021

/approve

@openshift-ci
Copy link

openshift-ci bot commented Sep 10, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: feloy

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 openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label Sep 10, 2021
@feloy
Copy link
Contributor

feloy commented Sep 10, 2021

The output of odo link -h is

Usage:
  odo link <operator-service-type>/<service-name> OR link <service> --component [component] OR link <component> --component [component] [flags]

I think the link <service> --component [component] part is not accurate anymore

@mohammedzee1000
Copy link
Contributor Author

The output of odo link -h is

Usage:
  odo link <operator-service-type>/<service-name> OR link <service> --component [component] OR link <component> --component [component] [flags]

I think the link <service> --component [component] part is not accurate anymore

Removing

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

sonarcloud bot commented Sep 13, 2021

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
6.4% 6.4% Duplication

@feloy
Copy link
Contributor

feloy commented Sep 13, 2021

/lgtm

Thanks @mohammedzee1000 for this cleanup

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Sep 13, 2021
@openshift-merge-robot openshift-merge-robot merged commit 72186ca into redhat-developer:main Sep 13, 2021
@rm3l rm3l added the area/refactoring Issues or PRs related to code refactoring label Jun 16, 2023
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. area/refactoring Issues or PRs related to code refactoring lgtm Indicates that a PR is ready to be merged. Required by Prow.
Projects
None yet
4 participants