Skip to content

Add service to component (odo add binding)#2803

Merged
rgrunber merged 1 commit intoredhat-developer:mainfrom
datho7561:2668-odo-add-binding
Jul 13, 2023
Merged

Add service to component (odo add binding)#2803
rgrunber merged 1 commit intoredhat-developer:mainfrom
datho7561:2668-odo-add-binding

Conversation

@datho7561
Copy link
Copy Markdown
Contributor

@datho7561 datho7561 commented Mar 20, 2023

Right click on a component in the components view and select "Bind Service" in order to bind the component to an Operator-backed service.

In order for this to work:

  • the service binding operator must be installed on the cluster
  • an operator that manages instances of services must be installed (I tested with the RHOAS operator)
  • there must be an instance of the service that is managed by the operator in the current project

Other things in this PR:

  • Add a walkthrough GIF of using the UI
  • Add a smoke test to check that the "Bind Service" context menu item exists
  • Fix the smoke tests. This includes fixing the existing test case for Test case covering creating a component from component's view #2780, even though we are going to rewrite that as a WebView UI soon. I did this, since for the "Bind Service" smoke test, we need a component present in the components view.

Closes #2668

Signed-off-by: David Thompson davthomp@redhat.com

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 20, 2023

Codecov Report

Patch coverage: 29.41% and project coverage change: -0.17 ⚠️

Comparison is base (7ad8679) 33.88% compared to head (79b038f) 33.72%.

❗ Current head 79b038f differs from pull request most recent head 694cf06. Consider uploading reports for the commit 694cf06 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2803      +/-   ##
==========================================
- Coverage   33.88%   33.72%   -0.17%     
==========================================
  Files          70       71       +1     
  Lines        4326     4395      +69     
  Branches      777      780       +3     
==========================================
+ Hits         1466     1482      +16     
- Misses       2860     2913      +53     
Impacted Files Coverage Δ
src/odo/command.ts 13.77% <0.00%> (-0.34%) ⬇️
src/odo3.ts 30.76% <13.33%> (-15.39%) ⬇️
...add-service-binding/addServiceBindingViewLoader.ts 17.24% <17.24%> (ø)
src/openshift/component.ts 15.42% <36.66%> (+0.11%) ⬆️
src/webview/devfile-registry/registryViewLoader.ts 31.11% <100.00%> (+2.22%) ⬆️

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@datho7561 datho7561 force-pushed the 2668-odo-add-binding branch 7 times, most recently from baa15ba to 119a03e Compare April 6, 2023 20:27
@datho7561 datho7561 force-pushed the 2668-odo-add-binding branch 3 times, most recently from 141bff4 to 694cf06 Compare April 13, 2023 14:18
@datho7561 datho7561 marked this pull request as ready for review April 13, 2023 14:40
@datho7561
Copy link
Copy Markdown
Contributor Author

@mohitsuman @msivasubramaniaan this PR is ready for review

@datho7561
Copy link
Copy Markdown
Contributor Author

datho7561 commented May 30, 2023

Need to add integration tests for odo add binding Okay I added them

@datho7561 datho7561 force-pushed the 2668-odo-add-binding branch from 619f49c to efecc5c Compare June 1, 2023 19:27
@datho7561 datho7561 marked this pull request as ready for review June 1, 2023 19:28
Copy link
Copy Markdown
Collaborator

@msivasubramaniaan msivasubramaniaan left a comment

Choose a reason for hiding this comment

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

Here are the comments which needs to be addressed in order to UI

  • The webview should align with VSCode theme in terms of button color, font size, font family
    image
  • Binding name accepts the invalid names as well
    image
  • In order to showing the error message while loading, we can make the button disabled and show the error message based on user input
  • Don't open multiple service binding webview for same component

@datho7561 datho7561 force-pushed the 2668-odo-add-binding branch from efecc5c to 0947c84 Compare June 7, 2023 19:53
@datho7561 datho7561 force-pushed the 2668-odo-add-binding branch from 0947c84 to fbe1c96 Compare June 22, 2023 12:58
Copy link
Copy Markdown
Member

@JessicaJHee JessicaJHee left a comment

Choose a reason for hiding this comment

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

Works well from my testing! Could consider adding a notification when the service was successfully binded (either in the webview and not have it close on it own, or a vscode window popup?), I wasn't sure if everything worked when the webview closed.

Another small thing with the add service binding button, could we keep that disabled but visible when the user hasn't picked the service or service name instead of replacing the error box? But it's not a big deal so if it takes a long time to implement then I don't think it's worth changing.

Comment thread src/openshift/component.ts Outdated
Comment thread src/webview/add-service-binding/app/addServiceBindingForm.tsx
Copy link
Copy Markdown
Collaborator

@msivasubramaniaan msivasubramaniaan left a comment

Choose a reason for hiding this comment

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

There was an error in dev console log while loading the Bind service webview
image

Comment thread src/webview/add-service-binding/app/addServiceBindingForm.tsx
Comment thread src/openshift/component.ts
@datho7561 datho7561 force-pushed the 2668-odo-add-binding branch 4 times, most recently from aaeddd6 to 13b65b3 Compare June 22, 2023 20:35
rgrunber
rgrunber previously approved these changes Jun 28, 2023
Copy link
Copy Markdown
Member

@rgrunber rgrunber left a comment

Choose a reason for hiding this comment

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

Works for me. We should merge and improve afterwards if there are any other issues.

@mohitsuman
Copy link
Copy Markdown
Contributor

@datho7561 have we added segment event to this command workflow. We need to update around:

  • User opens the webview
  • Submits the form

@datho7561 datho7561 force-pushed the 2668-odo-add-binding branch from d4bb85b to 45914b9 Compare July 4, 2023 19:26
@datho7561
Copy link
Copy Markdown
Contributor Author

I added two new events to track the usage of the "Bind Service" UI: startAddBindingWizard and finishAddBindingWizard. We also have the telemetry of when the command fails or is run successfully.

@datho7561
Copy link
Copy Markdown
Contributor Author

@msivasubramaniaan if you think it's good, could you please +1 when you have time (no rush)

datho7561 added a commit to datho7561/vscode-openshift-tools that referenced this pull request Jul 5, 2023
With a running `crc` cluster, run `npm run cluster-ui-test` in order to
run the cluster-dependent UI test suite.

- renamed `smoke-test` to `cluster-ui-test` and removed some of the
  tests that overlap between the suites
- Most of this work is also done in redhat-developer#2803, so I just copied it from over
  there
- Temporarily addresses redhat-developer#2780, we will need to rewrite it once the new
  create component workflow is in place

Fixes redhat-developer#2777

Signed-off-by: David Thompson <davthomp@redhat.com>
@datho7561
Copy link
Copy Markdown
Contributor Author

@rgrunber let me know if you think it's good. However, I think we are blocked on Muthu giving a +1 in order to merge the PR

datho7561 added a commit that referenced this pull request Jul 13, 2023
With a running `crc` cluster, run `npm run cluster-ui-test` in order to
run the cluster-dependent UI test suite.

- renamed `smoke-test` to `cluster-ui-test` and removed some of the
  tests that overlap between the suites
- Most of this work is also done in #2803, so I just copied it from over
  there
- Temporarily addresses #2780, we will need to rewrite it once the new
  create component workflow is in place

Fixes #2777

Signed-off-by: David Thompson <davthomp@redhat.com>
Right click on a component in the components view and select "Bind
Service" in order to bind the component to an Operator-backed service.

In order for this to work:
- the service binding operator must be installed on the cluster
- an operator that manages instances of services must be installed
  (I tested with the RHOAS operator)
- there must be an instance of the service that is managed by the operator in the current project

Other things in this PR:
- Add a walkthrough GIF of using the UI
- Add a smoke test to check that the "Bind Service" context menu item
  exists
- Fix the smoke tests.
  This includes fixing the existing test case for redhat-developer#2780,
  even though we are going to rewrite that as a WebView UI soon.
  I did this, since for the "Bind Service" smoke test,
  we need a component present in the components view.

Signed-off-by: David Thompson <davthomp@redhat.com>
@datho7561 datho7561 force-pushed the 2668-odo-add-binding branch from 45914b9 to 22df9bf Compare July 13, 2023 13:17
@rgrunber rgrunber added this to the 1.6.0 milestone Jul 13, 2023
@rgrunber rgrunber merged commit d5d3ff0 into redhat-developer:main Jul 13, 2023
@datho7561 datho7561 deleted the 2668-odo-add-binding branch July 13, 2023 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support Adding Service to component workflow

5 participants