Adds mapping support to odo link #5237

dharmit · 2021-11-22T09:46:21Z

Sticking to SBO library v0.9.0 because the feature can be implemented
using it just fine.
Docs doesn't cover --bind-as-files. I would prefer to keep it in a
separate PR. --bind-as-files docs e8dfa50.
Added id field to the ServiceBinding resource created by odo and it
defaults to same name as that of service's. No option to change the
id itself.
Removed a hyphen from the redis.yaml file because it caused odo push for a link using the --map flag to fail due to it.

What type of PR is this?
/kind feature

What does this PR do / why we need it:
$subject. We need it for the users to be able to inject custom binding data while linking

Which issue(s) this PR fixes:

Fixes #4937

PR acceptance criteria:

Unit test
Integration test
Documentation
I have read the test guidelines

How to test changes / Special notes to the reviewer:
Considering the backend component and postgres service from odo quickstart guide, try linking using:

$ odo link PostgresCluster/hippo --map pgVersion='{{ .database.spec.postgresVersion }}'
$ odo exec -- env | grep pgVersion

netlify · 2021-11-22T09:48:14Z

✔️ Deploy Preview for odo-docusaurus-preview canceled.

🔨 Explore the source changes: 6b1a7dc

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

dharmit · 2021-11-22T09:48:37Z

Sticking to SBO library v0.9.0 because the feature can be implemented using it just fine.

Another reason being that there are certain breaking changes which were taking longer to figure out. Refer discussion in this thread on Kubernetes Slack for more context.

feloy · 2021-11-23T15:02:26Z

docs/website/docs/command-reference/link.md

+sidebar_position: 4
+---
+
+`odo link` command helps link an odo component to an Operator backed service or another odo component. It does this by using [Service Binding Operator](https://github.com/redhat-developer/service-binding-operator). At the time of writing this, odo makes use of the Service Binding library and not the Operator itself to achieve the desired functionality.


This documentation is very detailed, and based on the quickstart project: I think it would be better to place it in the Using odo section, and have a more concise and generic doc as reference

Yes, while writing this, I had the thought of putting a concise form of it in the "Using odo" section. But can we do it separately?

Also, I think only this doc in "Command Reference" is based on Quickstart project. Which is not good. But I couldn't think of a better way to convey the point. Do you have any other idea?

Ah, I see your point now. You are suggesting to add this document to the "Using odo" section. And in this section, we keep a concise document.

We will have to add the bits for linking backend and frontend component in that case. And also figure out what to keep in what doc. Doable. Let's do it.

pkg/odo/cli/component/common_link.go

pkg/odo/util/cmdutils.go

kadel · 2021-11-26T09:31:12Z

I'm having some issues with keys referencing another types like for example

odo link PostgresCluster/hippo --map mydbname='path={.metadata.name}-pguser-{.metadata.name},objectType=Secret,sourceKey=dbname'

But I might be doing something wrong, or it might need newer SB version.
Simple values like mentioned in documentation worked for me, so we can figure out this in separate issue.

/approve

feloy · 2021-11-26T13:17:30Z

@dharmit I'm not sure that removing the -app at the end of the name is what we want. I'm pretty sure that this appname was missing before, because of a bug, bug that disappeared with the refactoring

We can see the same thing in the integration tests: some tests are checking that the name of some file does not contain app inside it, but I'm pretty sure we wanted to have it. With the refacto, the appname is now here, and the tests are failing.

dharmit · 2021-11-26T13:27:43Z

@feloy yeah, it's pretty annoying state that we are in right now. At the moment, the tests are failing (I'm checking locally as well against CRC) and here's the error message odo unlink prints if user tries to unlink something that isn't linked to it:

# from component directory of component A, link the component B
$ odo link compB
$ odo push

$ odo unlink compB
# try to unlink again
$ odo unlink compB
 ✗  failed to unlink the component "compB-app" since no link was found in the configuration referring this component

Expected error message is:

$ odo unlink compB
 ✗  failed to unlink the component "compB" since no link was found in the configuration referring this component

Now that we have modified o.serviceName to be the name of the Kubernetes Service belonging to the component, things are failing elsewhere. 😕

dharmit · 2021-11-26T13:28:54Z

One way to handle this better could be to introduce an element in the commonLinkOptions struct. I can't think of a better way at the moment.

dharmit · 2021-11-29T08:31:55Z

[ssh:Fedora-32-minikube] [odo]  ✓  Deleted link "link-dmwnmv" on the cluster; component will be restarted
[ssh:Fedora-32-minikube] [odo]  •  Restarting the component  ...
[ssh:Fedora-32-minikube] [odo]  ✗  Restarting the component [1m]
[ssh:Fedora-32-minikube] [odo]  ✗  Failed to start component with name "qjma". Error: Failed to create the component: timeout while waiting for "qjma-app-6785b7ff9f-mzzx7" pod to be deleted
[ssh:Fedora-32-minikube] Running odo with args [odo service delete Redis/flcqck -f]
[ssh:Fedora-32-minikube] Deleting project: cmd-service-test480dff

/test psi-kubernetes-integration-e2e

feloy · 2021-11-29T13:26:42Z

/lgtm

openshift-bot · 2021-11-29T13:30:23Z

/retest-required