Closed jfavellar90 closed 1 year ago
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. 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.
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. 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:
gh-pages
branch commitNow 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
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
@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:
helm repo remove openedx-harmony
). Then, follow the instructions to install the chart mentioned above.@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: