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

Add topology data model factory for bindable resources #9713

Merged

Conversation

rottencandy
Copy link
Contributor

Fixes:
https://issues.redhat.com/browse/ODC-6160

Description:

Contribute topology data model factory for bindable services as a dynamic extension.
List of bindable resources is fetched up front because topology extensions require list of resources to be already present.

Screen shots / Gifs for design review:
N/A

Unit test coverage report:

Browser conformance:

  • Chrome
  • Firefox
  • Safari
  • Edge

/kind feature

@openshift-ci openshift-ci bot added the kind/feature Categorizes issue or PR as related to a new feature. label Aug 3, 2021
@openshift-ci openshift-ci bot added the component/topology Related to topology label Aug 3, 2021
@rottencandy
Copy link
Contributor Author

/hold

This requires adding support for providing resources through a function instead of supplying the object.
Done in #9726

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 4, 2021
@rottencandy rottencandy changed the title Add topology data model factory for bindable resources [WIP] Add topology data model factory for bindable resources Aug 11, 2021
@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. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 11, 2021
@openshift-ci openshift-ci bot added component/dev-console Related to dev-console component/helm Related to helm-plugin component/kubevirt Related to kubevirt-plugin component/sdk Related to console-plugin-sdk and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 19, 2021
@rottencandy rottencandy force-pushed the feat/bindable-service branch 3 times, most recently from 8ebc522 to f80f3eb Compare August 19, 2021 15:26
@rottencandy rottencandy changed the title [WIP] Add topology data model factory for bindable resources Add topology data model factory for bindable resources Aug 19, 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. label Aug 19, 2021
@rottencandy
Copy link
Contributor Author

/hold cancel

Will be proceeding with fetching bindable service CR on page load, since redesigning dynamic extension requires additional effort.

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 19, 2021
kind: 'BindableService',
plural: 'BindableServices',
label: 'BindableService',
labelPlural: 'BindableServices',
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
labelPlural: 'BindableServices',
labelPlural: 'Bindable Services',

Copy link
Contributor

Choose a reason for hiding this comment

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

@sahil143 @rottencandy
The latest localization uses the same PascalCasing as the kind in the label. Have a look at https://github.com/openshift/console/blob/master/frontend/public/models/index.ts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted label & labelPlural to PascalCase.

@rottencandy rottencandy force-pushed the feat/bindable-service branch 2 times, most recently from 26437a2 to 69da08c Compare August 19, 2021 20:10
Copy link
Contributor

@sahil143 sahil143 left a comment

Choose a reason for hiding this comment

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

tested locally with dummy CRD and CR

kind: CustomResourceDefinition
apiVersion: apiextensions.k8s.io/v1
metadata:
  name: bindableservices.binding.operators.coreos.com
spec:
  group: config.openshift.io
  names:
    plural: bindableservices
    singular: bindableservice
    kind: BindableService
    listKind: BindableServiceList
  scope: Cluster
  versions:
    - name: v1alpha1
      served: true
      storage: true
      schema:
        openAPIV3Schema:
          description: >-
            Bindable Services demo. 
             Compatibility level 1: Stable within a major release for a minimum of 12 months or 3 minor releases (whichever is longer).
          type: object
          required:
            - spec
          properties:
            apiVersion:
              description: >-
                APIVersion defines the versioned schema of this representation
                of an object. Servers should convert recognized schemas to the
                latest internal value, and may reject unrecognized values. More
                info:
                https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources
              type: string
            kind:
              description: >-
                Kind is a string value representing the REST resource this
                object represents. Servers may infer this from the endpoint the
                client submits requests to. Cannot be updated. In CamelCase.
                More info:
                https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds
              type: string
            metadata:
              type: object
            spec:
              description: spec holds user settable values for configuration
              type: array
              items:
                type: object
                properties:
                  group:
                    type: string
                  version:
                    type: string
                  kind: 
                    type: string

Resource

apiVersion: binding.operators.coreos.com/v1alpha1
kind: BindableService
metadata:
  name: bindable-services
spec:
  - group: camel.apache.org
    kind: IntegrationPlatform
    version: v1

Bindable resource with external-service label
Kapture 2021-08-20 at 01 33 53

Bindable resource without an external-service label is not visualized
Kapture 2021-08-20 at 01 42 57

UI breaks when clicking on a Bindable node to open the Sidebar but it's being fixed in #9841
Screenshot 2021-08-20 at 1 35 33 AM

@sahil143
Copy link
Contributor

/label qe-approved

@openshift-ci openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Aug 19, 2021
@sahil143
Copy link
Contributor

/label px-approved docs-approved

Added label on behalf of doc, px since epic https://issues.redhat.com/browse/ODC-5722 has ack from docs and px and AC hasn't changed

Copy link
Contributor

@sahil143 sahil143 left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 20, 2021
@rohitkrai03
Copy link
Contributor

/label px-approved
/label docs-approved

Added label on behalf of doc, px since epic https://issues.redhat.com/browse/ODC-5722 has ack from docs and px and AC hasn't changed

@openshift-ci openshift-ci bot added px-approved Signifies that Product Support has signed off on this PR docs-approved Signifies that Docs has signed off on this PR labels Aug 20, 2021
@@ -0,0 +1,4 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this file since you're using devconsole for i18n strings now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 20, 2021
Copy link
Contributor

@rohitkrai03 rohitkrai03 left a comment

Choose a reason for hiding this comment

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

/approve

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 20, 2021
@sahil143
Copy link
Contributor

/lgtm

tested locally, works as expected

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 20, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 20, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rohitkrai03, rottencandy, sahil143

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

@sahil143
Copy link
Contributor

/retest

@sahil143
Copy link
Contributor

/test all

@openshift-merge-robot openshift-merge-robot merged commit bac561d into openshift:master Aug 21, 2021
@rottencandy rottencandy deleted the feat/bindable-service branch August 23, 2021 06:51
@spadgett spadgett added this to the v4.9 milestone Aug 30, 2021
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. component/dev-console Related to dev-console component/helm Related to helm-plugin component/kubevirt Related to kubevirt-plugin component/sdk Related to console-plugin-sdk component/topology Related to topology docs-approved Signifies that Docs has signed off on this PR kind/feature Categorizes issue or PR as related to a new feature. kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated lgtm Indicates that a PR is ready to be merged. px-approved Signifies that Product Support has signed off on this PR qe-approved Signifies that QE has signed off on this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants