Skip to content
This repository was archived by the owner on Jun 26, 2024. It is now read-only.

Conversation

Kartikey-star
Copy link
Contributor

This PR defines the Service Binding Operator Helm chart.

@Kartikey-star Kartikey-star force-pushed the helm-chart branch 2 times, most recently from 56873a4 to 018cbe0 Compare April 25, 2022 07:28
@Kartikey-star Kartikey-star requested a review from pmacik April 25, 2022 07:29
spec:
containers:
- name: "{{ .Release.Name }}-test"
image: "quay.io/kmamgain/sboat:latest"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please include the Dockerfile for this image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Dockerfile and shell script cannot be kept in the charts directory . Can u suggest some other directory to place those.

@Kartikey-star Kartikey-star force-pushed the helm-chart branch 2 times, most recently from a83011d to 0e390fa Compare April 28, 2022 10:00
@codecov
Copy link

codecov bot commented Apr 28, 2022

Codecov Report

Merging #1132 (730cdcc) into master (9b3a8ec) will not change coverage.
The diff coverage is n/a.

❗ Current head 730cdcc differs from pull request most recent head 5cc9331. Consider uploading reports for the commit 5cc9331 to get more accurate results

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1132   +/-   ##
=======================================
  Coverage   57.50%   57.50%           
=======================================
  Files          31       31           
  Lines        2673     2673           
=======================================
  Hits         1537     1537           
  Misses        974      974           
  Partials      162      162           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9b3a8ec...5cc9331. Read the comment docs.

@Kartikey-star Kartikey-star force-pushed the helm-chart branch 8 times, most recently from 6e2bdf0 to 0e8de4e Compare May 4, 2022 11:49
@Kartikey-star Kartikey-star force-pushed the helm-chart branch 3 times, most recently from f868eba to de48910 Compare May 10, 2022 13:26
@sadlerap
Copy link
Contributor

/retest

@Kartikey-star
Copy link
Contributor Author

/retest

Copy link
Contributor

@baijum baijum 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 lgtm and removed lgtm labels May 12, 2022
@dperaza4dustbit
Copy link
Contributor

/test 4.8-acceptance

@dperaza4dustbit
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm label May 16, 2022
description: A Helm chart to deploy service binding operator
type: application
version: 1.0.0
appVersion: "1.0.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

similar to OLM bundle, the chart needs to specify what is minimal supported kube version, add same set of keywords and other metadata available in OLM bundle.

/hold

Copy link
Contributor

Choose a reason for hiding this comment

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

kubeVersion: '>= 1.21.0' for OCP 4.8 right? https://access.redhat.com/solutions/4870701

Copy link
Contributor

Choose a reason for hiding this comment

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

CI tests with 1.19: https://github.com/redhat-developer/service-binding-operator/blob/master/.github/workflows/pr-checks.yaml#L13 so why putting the bar higher here? Also, there are plenty of useful metadata https://github.com/redhat-developer/service-binding-operator/blob/master/config/manifests/bases/service-binding-operator.clusterserviceversion.yaml that could be reused here as well, at least this is what I would expect from a chart carrying 1.0 version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Majority of data looks like field used by OLM ,as the target for our chart is non OLM i think it is fine just to mention the kubeversion.

Copy link
Contributor

Choose a reason for hiding this comment

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

In the chart you can find a few relevant counterparts, and the content that already exits for OLM could be transferred to Chart.yaml, for example:

  • icon
  • keywords
  • maintainer information
  • home
  • sources

If the idea is to publish this chart eventually to Artifacthub, you can also use a set of annotations supported by them to enrich the metadata.

@Kartikey-star
Copy link
Contributor Author

/retest

1 similar comment
@Kartikey-star
Copy link
Contributor Author

/retest

@pedjak
Copy link
Contributor

pedjak commented May 16, 2022

The chart should contain the appropriate license as well.

@Kartikey-star
Copy link
Contributor Author

/retest

@Kartikey-star
Copy link
Contributor Author

/retest

1 similar comment
@Kartikey-star
Copy link
Contributor Author

/retest

@Kartikey-star Kartikey-star force-pushed the helm-chart branch 3 times, most recently from fc0a528 to 2406f7a Compare May 17, 2022 04:33
@Kartikey-star
Copy link
Contributor Author

/retest

2 similar comments
@Kartikey-star
Copy link
Contributor Author

/retest

@Kartikey-star
Copy link
Contributor Author

/retest

Signed-off-by: Kartikey Mamgain <[email protected]>
@dperaza4dustbit
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm label May 17, 2022
@openshift-merge-robot openshift-merge-robot merged commit 93a55b4 into redhat-developer:master May 17, 2022
Copy link
Contributor

@Srivaralakshmi Srivaralakshmi left a comment

Choose a reason for hiding this comment

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

@Kartikey-star Left some review suggestions. PTAL and apply the changes in a different PR. Thanks!

- servicebindings.binding.operators.coreos.com
- servicebindings.servicebinding.io

The resources required for the Service Binding Operator will also be installed.
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
The resources required for the Service Binding Operator will also be installed.
The resources required for the Service Binding Operator are also installed.

- `image.testRepository`
- `keepTestResources`

A user can define values for the image PullPolicy.
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
A user can define values for the image PullPolicy.
You can define values for the image pull policy.

OR

Suggested change
A user can define values for the image PullPolicy.
You can define values for the `image.pullPolicy` value.

I am not sure about the technical accuracy of the previous sentence though. The 1st option that I have suggested makes more sense to me compared to the 2nd one.

- `keepTestResources`

A user can define values for the image PullPolicy.
A user can define values for `image.repository` and `image.testRepository`. If user is not able to pull image from quay.io registry, they can copy the image to their own container registry.
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
A user can define values for `image.repository` and `image.testRepository`. If user is not able to pull image from quay.io registry, they can copy the image to their own container registry.
You can define values for the `image.pullPolicy`, `image.repository`, and `image.testRepository` values. If you are not able to pull an image from the quay.io registry, then copy the image to your own container registry.

If combining both the sentences and saying that you can define values for all 3 values does not change the technical meaning, then you can implement the above suggestion. PTAL and decide.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

looks good to change.


A user can define values for the image PullPolicy.
A user can define values for `image.repository` and `image.testRepository`. If user is not able to pull image from quay.io registry, they can copy the image to their own container registry.
As part of Helm test we delete the deploymemt,service binding resource and secret used for testing. If a user is interested to view them, then he has to install the chart with keepTestResources set to `true`.
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
As part of Helm test we delete the deploymemt,service binding resource and secret used for testing. If a user is interested to view them, then he has to install the chart with keepTestResources set to `true`.
As part of the Helm test, objects such as deployment, service binding resources, and secrets used for testing are deleted. To view them, you can install the chart with the `keepTestResources` flag value set to `true`.

kubectl apply -f https://github.com/jetstack/cert-manager/releases/download/v1.6.0/cert-manager.yaml

### Adding the Helm chart repository
You need to add our helm repository to your local repository. Name the repository as per your convenience.
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
You need to add our helm repository to your local repository. Name the repository as per your convenience.
Add the `service-binding-operator-helm-chart` repository to your local repository and name the repository as per your convenience:


The previous output verifies that the `my-k-config` secret is created.

Run the Helm test (specify the namespace if applicable) using :
Copy link
Contributor

Choose a reason for hiding this comment

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

. Run the Helm test and specify the namespace if applicable:

----
The `Succeeded` phase from the output indicates that the Helm test has run successfully.

Verify that the Helm test has run successfully:
Copy link
Contributor

Choose a reason for hiding this comment

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

. Verify that the Helm test has run successfully:

service-binding-operator-release-test 0/1 Completed 0 4m28s
----

This implies that you have successfully installed the service binding operator using a Helm chart and are able to bind your workload to backing services.
Copy link
Contributor

Choose a reason for hiding this comment

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

The output verifies that you have successfully installed the {servicebinding-title} using a Helm chart and are able to bind your workload to backing services.


This implies that you have successfully installed the service binding operator using a Helm chart and are able to bind your workload to backing services.

Please ensure to delete the secret (specify the namespace if applicable) created :
Copy link
Contributor

Choose a reason for hiding this comment

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

. As a safety measure, delete the secret created and specify the namespace if applicable:

Please ensure to delete the secret (specify the namespace if applicable) created :
----
kubectl delete secret my-k-config --namespace service-binding-operator
----
Copy link
Contributor

Choose a reason for hiding this comment

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

After L240, add this:

.Example output
[source,terminal]

secret "my-k-config" deleted

The output verifies that the secret that you had created is now deleted.

Deleting the secret avoids exposing the secret credentials of the cluster to which you are connected.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants