-
Notifications
You must be signed in to change notification settings - Fork 14
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
Implementing releases for the harmony Helm Chart #40
Conversation
Thanks for the pull request, @jfavellar90! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
@jfavellar90 something is off with this PR. The certmanager, elasticseach, ingress and so on charts that were in .tgz form would be deleted. |
Now I went to look at the gh-pages branch and I see that those charts are dependencies now. Do we now for certain that the tar files that were in the repo were not modified and should be exact copies of what is in the dependencies repos? I mean, it should be like that, but I'd like to be sure that we are not requiring some custom change in them. |
@bradenmacdonald @felipemontoya I created a new PR and removed the one I created from Edunext's Harmony fork to prevent permissions problems when creating a new release in the repo. What's missing:
|
This is not a concern, we didn't have custom changes on those files, thanks to the releaser action we don't have to worry anymore about those dependencies artifacts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work @jfavellar90 !
I noticed one issue which is that the example helm install
command should specify a namespace like --namespace harmony
and in NOTES.txt
we need to change the namespace in the example command to use {{ .Release.Namespace }}
kubectl get svc -n harmony harmony-ingress-nginx-controller
should be
kubectl get svc -n {{ .Release.Namespace }} harmony-ingress-nginx-controller
Here are the full logs from my test. You can see there was an error because I installed it into the default namespace, but the printed instructions for checking the load balancer assume the harmony
namespace.
infra-example % helm repo add openedx-harmony https://openedx.github.io/openedx-k8s-harmony
"openedx-harmony" has been added to your repositories
infra-example % helm repo update
Hang tight while we grab the latest from your chart repositories...
...Successfully got an update from the "openedx-harmony" chart repository
...Successfully got an update from the "kubernetes-dashboard" chart repository
Update Complete. ⎈Happy Helming!⎈
infra-example % kubectl apply -f https://github.com/cert-manager/cert-manager/releases/download/v1.10.1/cert-manager.crds.yaml --namespace=harmony
...
infra-example % helm install harmony openedx-harmony/harmony-chart
NAME: harmony
LAST DEPLOYED: Tue Jun 13 15:20:03 2023
NAMESPACE: default
STATUS: deployed
REVISION: 1
TEST SUITE: None
NOTES:
Your harmony cluster is now ready to use!
An NGINX load balancer routes all HTTP/HTTPS traffic into the cluster to each
Open edX instance. Even before you deploy any Open edX instances, you can test
that the load balancer is working. First, get its external IP using
kubectl get svc -n harmony harmony-ingress-nginx-controller
Next, go to http://the.external.ip.shown/cluster-echo-test and make sure you get
a JSON response.
infra-example % kubectl get svc -n harmony harmony-ingress-nginx-controller
Error from server (NotFound): namespaces "harmony" not found
infra-example % kubectl get svc -n default harmony-ingress-nginx-controller
NAME TYPE CLUSTER-IP EXTERNAL-IP PORT(S) AGE
harmony-ingress-nginx-controller LoadBalancer 10.245.118.79 146.190.188.238 80:32179/TCP,443:30328/TCP 3m30s
Oh yeah, don't forget
|
f2882ae
to
1e4c0a3
Compare
1e4c0a3
to
74ee1e7
Compare
@bradenmacdonald I addressed your comments. I added the corresponding docs and recreated the Helm chart release to include the change in the
|
@bradenmacdonald please let me know if we are Ok to merge this PR since I need to revert some changes before hitting the merge button |
@jfavellar90 I tested it again with a fresh cluster on DigitalOcean and it worked perfectly. Please go ahead and revert those changes and merge :) 👍🏻 |
@jfavellar90 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
This PR aims to implement the chart-releaser action to handle the releases of the harmony chart, hosting it in GitHub Pages. This PR implies the following changes in this repo:
charts
, which complies with the chart-releaser standard.gh-pages
branch was pushed to the repo. This branch will host the README of the repo and anindex.yaml
file containing harmony chart helpful metadata to install the chart with helm. Thisindex.yaml
file is auto-generated by the chart-releaser action, so it will exist once the first release is run.gh-pages
branch, as indicated in the chart-releaser prerequisites. The page is already available at https://openedx.github.io/openedx-k8s-harmony/If this is merged, operators would be able to add the helm repo and install the chart like this: