Skip to content

Conversation

@divyanshiGupta
Copy link
Contributor

@divyanshiGupta divyanshiGupta commented Mar 9, 2022

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

This PR adds the fix for sorting catalog filters alphabetically and
uses useK8sWatchResource instead of k8sGet for fetching the bindable-services.

Screenshots:
Screenshot 2022-03-08 at 11 13 24 PM

Screenshot 2022-03-08 at 11 13 40 PM

Screenshot 2022-03-09 at 4 12 08 PM

Screenshot 2022-03-09 at 4 19 34 PM

/kind feature

@openshift-ci openshift-ci bot added the kind/feature Categorizes issue or PR as related to a new feature. label Mar 9, 2022
@divyanshiGupta
Copy link
Contributor Author

@beaumorley can you PTAL at this PR and approve if the badge and filters looks good to you?

@openshift-ci openshift-ci bot added component/dev-console Related to dev-console component/shared Related to console-shared labels Mar 9, 2022
@divyanshiGupta divyanshiGupta force-pushed the sb-discoverability branch 3 times, most recently from a8f51f4 to 26cbc58 Compare March 9, 2022 11:04
@divyanshiGupta
Copy link
Contributor Author

/cc @christianvogt
/cc @invincibleJai

@divyanshiGupta
Copy link
Contributor Author

/retest

@christianvogt
Copy link
Contributor

/approve

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 9, 2022
@christianvogt
Copy link
Contributor

@beaumorley you'll see how there is a bindable as well as not bindable filter in the catalog list. This is the way the filters currently work in that all items must be covered by at least one filter. If we want to change this for boolean type filters, then we should raise a new issue with the details and not hold up this one.

Copy link
Member

@christoph-jerolimov christoph-jerolimov left a comment

Choose a reason for hiding this comment

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

Get this react error and then a white page when open the OperatorBackedService catalog with this PR. Can you take a look?

Error
Maximum update depth exceeded. This can happen when a component repeatedly calls setState inside componentWillUpdate or componentDidUpdate. React limits the number of nested updates to prevent infinite loops.
Call Stack
 checkForNestedUpdates
  vendors~main-fde4aab59bedd45747de.js:465232:15
 scheduleUpdateOnFiber
  vendors~main-fde4aab59bedd45747de.js:463264:3

/hold

@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 Mar 9, 2022
Copy link
Member

@christoph-jerolimov christoph-jerolimov left a comment

Choose a reason for hiding this comment

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

Some more requests to add some missing types.

@serenamarie125
Copy link
Contributor

Couple comments here

  1. I think we need to reconsider the filter "text" here. Let's cover it in the Friday call? (this shouldn't hold up merging this PR though, we can have a follow PR for text as needed)
  2. Shouldn't there be more spacing under the Service Binding title?

Otherwise looks great! FYI @slemeur

@divyanshiGupta
Copy link
Contributor Author

Get this react error and then a white page when open the OperatorBackedService catalog with this PR. Can you take a look?

Error
Maximum update depth exceeded. This can happen when a component repeatedly calls setState inside componentWillUpdate or componentDidUpdate. React limits the number of nested updates to prevent infinite loops.
Call Stack
 checkForNestedUpdates
  vendors~main-fde4aab59bedd45747de.js:465232:15
 scheduleUpdateOnFiber
  vendors~main-fde4aab59bedd45747de.js:463264:3

/hold

@jerolimov I couldn't reproduce this issue. Can you share the steps to reproduce this?

Screen.Recording.2022-03-10.at.4.51.35.PM.mov

@divyanshiGupta
Copy link
Contributor Author

Couple comments here

  1. I think we need to reconsider the filter "text" here. Let's cover it in the Friday call? (this shouldn't hold up merging this PR though, we can have a follow PR for text as needed)
  2. Shouldn't there be more spacing under the Service Binding title?

Otherwise looks great! FYI @slemeur

@serenamarie125 this is how it looks like for other filters as well. We can have a separate issue for this.
Screenshot 2022-03-10 at 4 58 54 PM

@divyanshiGupta
Copy link
Contributor Author

/retest

1 similar comment
@divyanshiGupta
Copy link
Contributor Author

/retest

Copy link
Contributor

@serenamarie125 serenamarie125 left a comment

Choose a reason for hiding this comment

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

great work!

@christoph-jerolimov
Copy link
Member

/retest

@christoph-jerolimov
Copy link
Member

The updated flags names fix the crash when the operator is not installed.
/unhold

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Mar 11, 2022
@christoph-jerolimov
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 11, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Mar 11, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, divyanshiGupta, jerolimov, serenamarie125

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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-bot
Copy link
Contributor

/retest-required

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

10 similar comments
@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@invincibleJai
Copy link
Member

/label docs-approved
/label px-approved

propagating above labels based on ACKs on epic

cc @rishumehra

@openshift-ci openshift-ci bot added docs-approved Signifies that Docs has signed off on this PR px-approved Signifies that Product Support has signed off on this PR labels Mar 14, 2022
@christoph-jerolimov
Copy link
Member

  1. Shouldn't there be more spacing under the Service Binding title?

@divyanshiGupta FYI: I created this issue and PR for the PF catalog extension.

@christoph-jerolimov
Copy link
Member

christoph-jerolimov commented Mar 14, 2022

Tested it on a cluster bot instance, with and without Service Binding Operator, without and with Postgres for Kubernetes operator. As kubeadmin and user with limited access.

/label qe-approved

@openshift-ci openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Mar 14, 2022
@openshift-bot
Copy link
Contributor

/retest-required

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

6 similar comments
@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Mar 15, 2022

@divyanshiGupta: all tests passed!

Full PR test history. Your PR dashboard.

Details

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.

@openshift-merge-robot openshift-merge-robot merged commit 25f332c into openshift:master Mar 15, 2022
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/shared Related to console-shared docs-approved Signifies that Docs has signed off on this PR kind/feature Categorizes issue or PR as related to a new feature. 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.

7 participants