Skip to content
This repository has been archived by the owner on Nov 18, 2020. It is now read-only.

Upgrade helm sample to 0.11.0 version of SDK #78

Merged
merged 5 commits into from
Oct 24, 2019
Merged

Upgrade helm sample to 0.11.0 version of SDK #78

merged 5 commits into from
Oct 24, 2019

Conversation

camilamacedo86
Copy link
Contributor

@camilamacedo86 camilamacedo86 commented Oct 12, 2019

Description

  • Update project to work with SDK version 0.11.0
  • Add README and Makefile in order to help users know how to check/use it

@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 12, 2019
Copy link

@jmccormick2001 jmccormick2001 left a comment

Choose a reason for hiding this comment

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

I built it locally, followed the README and it seems to work for me as suggested by the README

@joelanford
Copy link
Member

@camilamacedo86 now that the helm-operator supports pulling and using a helm-chart from a helm repo, I'd suggest we update this to use the stable/memcached helm chart. WDYT?

operator-sdk new memcached-operator --type=helm --helm-chart=stable/memcached

@camilamacedo86
Copy link
Contributor Author

I will check that @joelanford tks.

@openshift-ci-robot openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 21, 2019
helm/README.md Outdated
This directory contains samples of operators powered by Helm built using the [operator-sdk][operator_sdk]. To learn more about creating an operator that leverages Helm, check out the [Helm user guide][helm_user_guide].

[operator_sdk]:https://github.com/coreos/operator-sdk
[helm_user_guide]:https://github.com/operator-framework/operator-sdk/blob/master/doc/helm/user-guide.md
Copy link
Member

Choose a reason for hiding this comment

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

Any reason we're deleting this? Seems to have some good info and doesn't require someone to navigate all the way into the memcached-operator sample.

Also, are there any incoming links to this README from any other repo?

Copy link
Contributor Author

@camilamacedo86 camilamacedo86 Oct 23, 2019

Choose a reason for hiding this comment

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

Excellent catch. it was removed by mistake. back now.
Tks

helm/memcached-operator/README.md Outdated Show resolved Hide resolved
helm/memcached-operator/README.md Outdated Show resolved Hide resolved
Comment on lines 41 to 46
```
# Update the operator manifest to use the built image name (if you are performing these steps on OSX, see note below)
$ sed -i 's|REPLACE_IMAGE|quay.io/example-inc/memcached-operator|g' deploy/operator.yaml
# On OSX use:
$ sed -i "" 's|REPLACE_IMAGE|quay.io/example-inc/memcached-operator|g' deploy/operator.yaml
```
Copy link
Member

Choose a reason for hiding this comment

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

The formatting here isn't great. I'd suggest this:

Update the operator manifest to use the built image name (if you are performing these steps on OSX, see note below)
```sh
$ sed -i 's|REPLACE_IMAGE|quay.io/example/memcached-operator|g' deploy/operator.yaml
```

**Note**
If you are performing these steps on OSX, use the following `sed` command instead:
```sh
$ sed -i "" 's|REPLACE_IMAGE|quay.io/example/memcached-operator|g' deploy/operator.yaml
```

helm/memcached-operator/README.md Outdated Show resolved Hide resolved
helm/memcached-operator/README.md Outdated Show resolved Hide resolved
helm/memcached-operator/deploy/role.yaml Show resolved Hide resolved

Run `make install` to install the operator. Check that the operator is running in the cluster, also check that the example Memcached service was deployed.

Following the expected result.
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
Following the expected result.
Run the following commands to verify that the installation was successful:


### Uninstalling

To uninstall all that was performed in the above step run `make uninstall`.
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
To uninstall all that was performed in the above step run `make uninstall`.
To uninstall all that was performed in the above step, run `make uninstall`.

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 replaced the order to be easier the understanding and solve this issue 👍

@camilamacedo86
Copy link
Contributor Author

HI @joelanford,

Thank you for the review. All requests are done. Also, I retest it locally as follows. Feel free to use cmacedo/memcached-operator:helm if you wish.

$ kubectl get all -n helm-memcached
NAME                                      READY   STATUS    RESTARTS   AGE
pod/example-memcached-0                   1/1     Running   0          27s
pod/example-memcached-1                   1/1     Running   0          19s
pod/example-memcached-2                   1/1     Running   0          10s
pod/memcached-operator-555bb889b6-qs9m2   1/1     Running   0          32s

NAME                                 TYPE        CLUSTER-IP       EXTERNAL-IP   PORT(S)             AGE
service/example-memcached            ClusterIP   None             <none>        11211/TCP           27s
service/memcached-operator-metrics   ClusterIP   10.107.196.216   <none>        8686/TCP,8383/TCP   28s

NAME                                 READY   UP-TO-DATE   AVAILABLE   AGE
deployment.apps/memcached-operator   1/1     1            1           32s

NAME                                            DESIRED   CURRENT   READY   AGE
replicaset.apps/memcached-operator-555bb889b6   1         1         1       32s

NAME                                 READY   AGE
statefulset.apps/example-memcached   3/3     27s 

Copy link
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

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

LGTM

@camilamacedo86 camilamacedo86 merged commit eefb019 into operator-framework:master Oct 24, 2019
@camilamacedo86 camilamacedo86 deleted the updat-helm branch October 24, 2019 10:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants