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 documentation on connecting to a service #6114

Merged

Conversation

valaparthvi
Copy link
Contributor

Signed-off-by: Parthvi Vala pvala@redhat.com

What type of PR is this:
/kind documentation
/area documentation

What does this PR do / why we need it:

Which issue(s) this PR fixes:

Fixes #6077

PR acceptance criteria:

  • Unit test

  • Integration test

  • Documentation

How to test changes / Special notes to the reviewer:

@netlify
Copy link

netlify bot commented Sep 12, 2022

Deploy Preview for odo-docusaurus-preview ready!

Name Link
🔨 Latest commit 045560b
🔍 Latest deploy log https://app.netlify.com/sites/odo-docusaurus-preview/deploys/632abbc7b932120008abce47
😎 Deploy Preview https://deploy-preview-6114--odo-docusaurus-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@openshift-ci openshift-ci bot added kind/documentation area/documentation Issues or PRs related to documentation or the 'odo.dev' website labels Sep 12, 2022
@openshift-ci openshift-ci bot requested review from dharmit and feloy September 12, 2022 11:18
@odo-robot
Copy link

odo-robot bot commented Sep 12, 2022

Unit Tests on commit fb4af19 finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Sep 12, 2022

Validate Tests on commit fb4af19 finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Sep 12, 2022

OpenShift Tests on commit fb4af19 finished with errors.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Sep 12, 2022

Kubernetes Tests on commit fb4af19 finished with errors.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Sep 12, 2022

Windows Tests (OCP) on commit fb4af19 finished successfully.
View logs: TXT HTML

@valaparthvi
Copy link
Contributor Author

======================
Unexpected error:
<*xpty.errPassthroughTimeout | 0xc002e32010>: {
error: <*errors.errorString | 0xc002e32000>{
s: "passthrough i/o timeout",
},
}
passthrough i/o timeout
occurred
In [It] at: /go/odo_1/tests/helper/helper_interactive.go:107

IBM OC tests are encountering this failure.

Windows tests are also facing some issue related to serviceaccount. See logs.
cc: @rm3l tagging you in case you see this error some place else.

/override OpenShift-Integration-tests/OpenShift-Integration-tests
/override windows-integration-test/Windows-test

@openshift-ci
Copy link

openshift-ci bot commented Sep 12, 2022

@valaparthvi: Overrode contexts on behalf of valaparthvi: OpenShift-Integration-tests/OpenShift-Integration-tests, windows-integration-test/Windows-test

In response to this:

======================
Unexpected error:
<*xpty.errPassthroughTimeout | 0xc002e32010>: {
error: <*errors.errorString | 0xc002e32000>{
s: "passthrough i/o timeout",
},
}
passthrough i/o timeout
occurred
In [It] at: /go/odo_1/tests/helper/helper_interactive.go:107

IBM OC tests are encountering this failure.

Windows tests are also facing some issue related to serviceaccount. See logs.
cc: @rm3l tagging you in case you see this error some place else.

/override OpenShift-Integration-tests/OpenShift-Integration-tests
/override windows-integration-test/Windows-test

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.

```

### Exit and cleanup
Press `Ctrl+c` to exit `odo dev`.
Copy link
Member

Choose a reason for hiding this comment

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

This is already in the output? Perhaps we don't need this in the doc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's alright to be explicit here before user proceeds with the next step.

valaparthvi and others added 9 commits September 13, 2022 14:54
Signed-off-by: Parthvi Vala <pvala@redhat.com>
Co-authored-by: Charlie Drage <charlie@charliedrage.com>
Signed-off-by: Parthvi Vala <pvala@redhat.com>
Signed-off-by: Parthvi Vala <pvala@redhat.com>
Update docs/website/docs/user-guides/advanced/_create-mongodb-service.mdx

Co-authored-by: Charlie Drage <charlie@charliedrage.com>
Signed-off-by: Parthvi Vala <pvala@redhat.com>
Signed-off-by: Parthvi Vala <pvala@redhat.com>
@valaparthvi valaparthvi force-pushed the connecting-service-doc branch from 37769db to 0a2128d Compare September 13, 2022 10:01
Copy link
Member

@cdrage cdrage left a comment

Choose a reason for hiding this comment

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

Sorry I reviewed before changes have been made! Noticed half way through the review. I'll submit these comments and review the rest at a later time.


### Implement the code logic
:::note
If you're already running `odo dev` in a terminal, exit it and start afresh.
Copy link
Member

Choose a reason for hiding this comment

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

That makes sense. I reccomend we remove all informal language however like "you're", so I reccomend this change:


2. [Install Percona Server Mongodb Operator via Operator Hub](https://operatorhub.io/operator/percona-server-mongodb-operator).
:::note
The operator will be installed in a new namespace called "my-percona-server-mongodb-operator" and will be usable from that namespace only.
Copy link
Member

Choose a reason for hiding this comment

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

Won't this mess up deployment with the service if it's only usable in that namespace?

Maybe we should add instructions to deploy / use this Go application in the same namespace as the operator?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just realized that we don't need to. A service deployed in any namespace is still accessible to the Go application deployed in a different namespace.

valaparthvi and others added 2 commits September 14, 2022 10:08
Co-authored-by: Charlie Drage <charlie@charliedrage.com>
Co-authored-by: Charlie Drage <charlie@charliedrage.com>
valaparthvi and others added 2 commits September 14, 2022 10:28
Co-authored-by: Charlie Drage <charlie@charliedrage.com>
Signed-off-by: Parthvi Vala <pvala@redhat.com>
@valaparthvi valaparthvi requested review from cdrage and rm3l September 14, 2022 08:53
Copy link
Member

@rm3l rm3l 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. Required by Prow. label Sep 14, 2022
@rm3l
Copy link
Member

rm3l commented Sep 15, 2022

Running oc with args [oc get csv -o jsonpath={.items[?(@.status.phase=="Succeeded")].metadata.name}]
    [oc] cloud-native-postgresql.v1.16.2 redis-operator.v0.8.0 service-binding-operator.v1.1.1Running oc with args [oc get csv -o jsonpath={.items[?(@.status.phase=="Succeeded")].metadata.name}]
    [oc] cloud-native-postgresql.v1.16.2 redis-operator.v0.8.0 service-binding-operator.v1.1.1Running oc with args [oc get bindablekinds bindable-kinds -ojsonpath={.status[*].kind}]
    [oc] Redis ClusterRunning oc with args [oc apply -f /go/odo_1/tests/examples/manifests/bindablekind-instance.yaml]
    [oc] Error from server (InternalError): error when creating "/go/odo_1/tests/examples/manifests/bindablekind-instance.yaml": Internal error occurred: failed calling webhook "vcluster.kb.io": Post "https://postgresql-operator-controller-manager-1-16-2-service.openshift-operators.svc:443/validate-postgresql-k8s-enterprisedb-io-v1-cluster?timeout=10s": EOF
    Deleting project: interactive-add-binding-test588iho

/override OpenShift-Integration-tests/OpenShift-Integration-tests

@openshift-ci
Copy link

openshift-ci bot commented Sep 15, 2022

@rm3l: Overrode contexts on behalf of rm3l: OpenShift-Integration-tests/OpenShift-Integration-tests

In response to this:

Running oc with args [oc get csv -o jsonpath={.items[?(@.status.phase=="Succeeded")].metadata.name}]
   [oc] cloud-native-postgresql.v1.16.2 redis-operator.v0.8.0 service-binding-operator.v1.1.1Running oc with args [oc get csv -o jsonpath={.items[?(@.status.phase=="Succeeded")].metadata.name}]
   [oc] cloud-native-postgresql.v1.16.2 redis-operator.v0.8.0 service-binding-operator.v1.1.1Running oc with args [oc get bindablekinds bindable-kinds -ojsonpath={.status[*].kind}]
   [oc] Redis ClusterRunning oc with args [oc apply -f /go/odo_1/tests/examples/manifests/bindablekind-instance.yaml]
   [oc] Error from server (InternalError): error when creating "/go/odo_1/tests/examples/manifests/bindablekind-instance.yaml": Internal error occurred: failed calling webhook "vcluster.kb.io": Post "https://postgresql-operator-controller-manager-1-16-2-service.openshift-operators.svc:443/validate-postgresql-k8s-enterprisedb-io-v1-cluster?timeout=10s": EOF
   Deleting project: interactive-add-binding-test588iho

/override OpenShift-Integration-tests/OpenShift-Integration-tests

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.

:::note
The operator will be installed in a new namespace called "my-percona-server-mongodb-operator" and the service can only be deployed in this namespace.
:::
3. Create a MongoDB service. This service has been created with a minimal
Copy link
Member

Choose a reason for hiding this comment

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

??? sentence cuts off

```shell
odo add binding \
--service mongodb-instance/PerconaServerMongoDB \
--service-namespace=my-percona-server-mongodb-operator \
Copy link
Member

Choose a reason for hiding this comment

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

I don't like the fact that we have to specify this namespace throughout the guide? We should add this to the beginning of the guide / prerequisites that we can specify our own too. See other comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we're specifying it throughout the doc. It's only for this command, and while creating mongodb instance because that is the only namespace this CRD is accessible from.


2. [Install Percona Server Mongodb Operator via Operator Hub](https://operatorhub.io/operator/percona-server-mongodb-operator).
:::note
The operator will be installed in a new namespace called "my-percona-server-mongodb-operator" and the service can only be deployed in this namespace.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The operator will be installed in a new namespace called "my-percona-server-mongodb-operator" and the service can only be deployed in this namespace.
The operator will be installed in a new namespace called "my-percona-server-mongodb-operator". the service can only be deployed in this namespace.

Still we don't explain why this namespace :(, bit confusing IMO.

I think we should actually show to use kubectl commands? Because then we can specify what namespace to install this.


2. [Install Percona Server Mongodb Operator via Operator Hub](https://operatorhub.io/operator/percona-server-mongodb-operator).

Install to the same namespace as your component:

kubectl create -n mynamespace -f https://operatorhub.io/install/percona-server-mongodb-operator.yaml

Obviously not "mynamespace" but something else?

This explains to the user that the service will be in a specific namespace, etc at the beginning of the doc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Still we don't explain why this namespace :(, bit confusing IMO.

Oh. This namespace is created when you install the mongodb operator. But if it is confusing, I guess I could make the statements more verbose.
Screenshot from 2022-09-15 20-32-03

kubectl create -n mynamespace -f https://operatorhub.io/install/percona-server-mongodb-operator.yaml

this does not work because the targetNamespace for the operator is set to the one that it creates at the beginning.


// Connection URI
var (
USERNAME = os.Getenv("PERCONASERVERMONGODB_MONGODB_USER_ADMIN_USER")
Copy link
Member

Choose a reason for hiding this comment

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

Even I didn't realize that it actually shows the environment variable.. how could a new user know either when this doc is in an initial user guide?

A user can install mongodb operator and then realize after "what are the env variables again I should use?"

I think it's important to describe before this step that we should explain how these environment variables are retrieved / added under prerequisites.

Example, for Step 4 in prerequisites, we should describe how we are able to find out all these variables that can be used in your application.

## Step 5. Exit and cleanup
Press `Ctrl+c` to exit `odo dev`.

Delete the MongoDB instance that we had created.
Copy link
Member

Choose a reason for hiding this comment

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

Stupid question, but why are we using kubectl / oc commands for deleting this mongodb instance?

Because odo delete binding isn't added yet right?

We should add a note that we intent on implementing this so we update our documentation accordingly in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not think we intend on implementing odo delete binding. We already have odo remove binding that will remove the binding from devfile.yaml, and eventually it will delete the ServiceBinding instance from cluster, but the mongodb instance stays the same.
Since we created the mongodb instance at the beginning, it's only fair that we remove it.

Signed-off-by: Parthvi Vala <pvala@redhat.com>
@valaparthvi valaparthvi requested a review from cdrage September 20, 2022 12:57
Copy link
Member

@cdrage cdrage left a comment

Choose a reason for hiding this comment

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

/approve

Thanks for all the changes and being patient with my reviews! Looks great 💯

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Sep 20, 2022
@openshift-ci
Copy link

openshift-ci bot commented Sep 20, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cdrage, rm3l

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 removed the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Sep 21, 2022
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

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
0.4% 0.4% Duplication

@valaparthvi
Copy link
Contributor Author

/lgtm
I had to push a minor change.

@openshift-ci
Copy link

openshift-ci bot commented Sep 21, 2022

@valaparthvi: you cannot LGTM your own PR.

In response to this:

/lgtm
I had to push a minor change.

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.

@feloy
Copy link
Contributor

feloy commented Sep 21, 2022

/lgtm

@feloy
Copy link
Contributor

feloy commented Sep 21, 2022

/override Kubernetes-Integration-Tests/Kubernetes-Integration-Tests
/override OpenShift-Integration-tests/OpenShift-Integration-tests

Doc only

@openshift-ci
Copy link

openshift-ci bot commented Sep 21, 2022

@feloy: Overrode contexts on behalf of feloy: Kubernetes-Integration-Tests/Kubernetes-Integration-Tests, OpenShift-Integration-tests/OpenShift-Integration-tests

In response to this:

/override Kubernetes-Integration-Tests/Kubernetes-Integration-Tests
/override OpenShift-Integration-tests/OpenShift-Integration-tests

Doc only

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.

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Sep 21, 2022
@openshift-merge-robot openshift-merge-robot merged commit 726ffc6 into redhat-developer:main Sep 21, 2022
kadel pushed a commit to kadel/odo that referenced this pull request Oct 3, 2022
* Add documentation to connect to a service

Signed-off-by: Parthvi Vala <pvala@redhat.com>

* Update docs/website/docs/user-guides/advanced/connecting-a-service.md

Co-authored-by: Armel Soro <armel@rm3l.org>

* Armel's review

* Update docs/website/docs/user-guides/advanced/connecting-a-service.md

Co-authored-by: Charlie Drage <charlie@charliedrage.com>

* Toggle output

Signed-off-by: Parthvi Vala <pvala@redhat.com>

* fix case for MongoDB

Signed-off-by: Parthvi Vala <pvala@redhat.com>

* Update docs/website/docs/user-guides/advanced/connecting-a-service.md

Update docs/website/docs/user-guides/advanced/_create-mongodb-service.mdx

Co-authored-by: Charlie Drage <charlie@charliedrage.com>

* Add Steps

Signed-off-by: Parthvi Vala <pvala@redhat.com>

* Add changes based on Charlie's review

Signed-off-by: Parthvi Vala <pvala@redhat.com>

* Update docs/website/docs/user-guides/advanced/connecting-a-service.md

Co-authored-by: Charlie Drage <charlie@charliedrage.com>

* Update docs/website/docs/user-guides/advanced/connecting-a-service.md

Co-authored-by: Charlie Drage <charlie@charliedrage.com>

* Update docs/website/docs/user-guides/advanced/connecting-a-service.md

Co-authored-by: Charlie Drage <charlie@charliedrage.com>

* more changes

Signed-off-by: Parthvi Vala <pvala@redhat.com>

* Finalize doc

Signed-off-by: Parthvi Vala <pvala@redhat.com>

* Add missing flag in command

Signed-off-by: Parthvi Vala <pvala@redhat.com>
Co-authored-by: Armel Soro <armel@rm3l.org>
Co-authored-by: Charlie Drage <charlie@charliedrage.com>
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/documentation Issues or PRs related to documentation or the 'odo.dev' website lgtm Indicates that a PR is ready to be merged. Required by Prow.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add documentation on connecting a service
5 participants