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

Implementing releases for the harmony Helm Chart #40

Merged
merged 6 commits into from
Jun 27, 2023

Conversation

jfavellar90
Copy link
Contributor

@jfavellar90 jfavellar90 commented Jun 13, 2023

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:

  • The harmony chart was moved to a folder called charts, which complies with the chart-releaser standard.
  • The gh-pages branch was pushed to the repo. This branch will host the README of the repo and an index.yaml file containing harmony chart helpful metadata to install the chart with helm. This index.yaml file is auto-generated by the chart-releaser action, so it will exist once the first release is run.
  • Repo configuration was changed to build GitHub pages from the gh-pages branch, as indicated in the chart-releaser prerequisites. The page is already available at https://openedx.github.io/openedx-k8s-harmony/
  • A GitHub Action was added to run the chart release routine. This action will be run only if changes in the charts are detected.

If this is merged, operators would be able to add the helm repo and install the chart like this:

helm repo add openedx-harmony https://openedx.github.io/openedx-k8s-harmony
helm repo update
helm install [RELEASE_NAME] openedx-harmony/harmony-chart

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Jun 13, 2023
@openedx-webhooks
Copy link

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:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

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 jfavellar90 changed the title Jhony/harmony chart releaser Implementing releases for the harmony Helm Chart Jun 13, 2023
@felipemontoya
Copy link
Member

@jfavellar90 something is off with this PR. The certmanager, elasticseach, ingress and so on charts that were in .tgz form would be deleted.
I'm all for having then in a format that is not binary but I don't think that deleting them should be a side effect.

@felipemontoya
Copy link
Member

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.

@jfavellar90
Copy link
Contributor Author

jfavellar90 commented Jun 13, 2023

@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.
I tested the release process by running the action on PR modification. The GH action created a new release you can check in the releases page. It contains the Helm artifact used to install the chart. Additionally, the GH page was released at https://openedx.github.io/openedx-k8s-harmony/ and now it's possible to install the chart via Helm using the commands indicated in the PR description.

What's missing:

  • Revert the change that triggers the action on PR update
  • Remove testing release and latest gh-pages branch commit
  • Add the docs on how to install the chart via Helm

@jfavellar90
Copy link
Contributor Author

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.

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

Copy link
Contributor

@bradenmacdonald bradenmacdonald left a 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

.gitignore Show resolved Hide resolved
@bradenmacdonald
Copy link
Contributor

Oh yeah, don't forget values.yaml - so the full example install command should probably be something like:

helm install harmony --namespace harmony --values values.yaml openedx-harmony/harmony-chart

@itsjeyd itsjeyd added the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Jun 15, 2023
@jfavellar90 jfavellar90 force-pushed the Jhony/harmony_chart_releaser branch 5 times, most recently from f2882ae to 1e4c0a3 Compare June 26, 2023 23:17
@jfavellar90
Copy link
Contributor Author

@bradenmacdonald I addressed your comments. I added the corresponding docs and recreated the Helm chart release to include the change in the NOTES.txt file. Feel free to test again following the new instructions. To prevent errors, I suggest:

  • Remove the current chart installation from your testing cluster.
  • Remove the helm repo (helm repo remove openedx-harmony). Then, follow the instructions to install the chart mentioned above.

@itsjeyd itsjeyd added waiting for eng review PR is ready for review. Review and merge it, or suggest changes. and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Jun 27, 2023
@jfavellar90
Copy link
Contributor Author

@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

@bradenmacdonald
Copy link
Contributor

bradenmacdonald commented Jun 27, 2023

@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 jfavellar90 merged commit 7f68330 into main Jun 27, 2023
@openedx-webhooks
Copy link

@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.

@jfavellar90 jfavellar90 deleted the Jhony/harmony_chart_releaser branch June 27, 2023 22:57
@itsjeyd itsjeyd removed the waiting for eng review PR is ready for review. Review and merge it, or suggest changes. label Jul 5, 2023
@itsjeyd itsjeyd added the core contributor PR author is a Core Contributor (who may or may not have write access to this repo). label Jul 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core contributor PR author is a Core Contributor (who may or may not have write access to this repo). open-source-contribution PR author is not from Axim or 2U
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants