-
Notifications
You must be signed in to change notification settings - Fork 1
Updates needed to start using this fork with Helm #77
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
base: master
Are you sure you want to change the base?
Conversation
A change to documentation files was detected in your PR. Please visit this link to preview changes: https://portal-staging.powerapp.cloud/docs?filters[kind]=all&filters[user]=all&filters[namespaceFilter]=master |
Hey @indiebrain, maybe you can take a look at this PR? 🙏😊 |
Hi @rurkss, maybe you could take a look at this, please? |
@gdubicki , apologies. We've not yet seen much community engagement and this slipped through the cracks. We'll discuss internally about how to do a better job of being more timely with reviews. Rest assured I've added this to the list of TODOs and we'll get a review here, soon. |
In the meantime, mind adding some context to the PR description what exactly the requirements are the necessitate these changes? We'll probably want to add some more specific contributing guides to this repo, but in general Power aims for context-rich review culture. Any additional details about what goals you're trying to reach, what specific problem(s) you need to solve, alternate approaches that were considered, outstanding questions, etc are extremely helpful for reviewers. Some of the sources from which we draw inspiration: |
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.
Thanks for the contribution!
If I'm reading between the lines, the goal of this PR is to make it possible to install the redis-operator via helm. Full disclosure, Power does NOT deploy our redis-operators via Helm. The release workflows we've adopted have neglected the Helm side of the world.
I'm not opposed to making this operator more accessible to those who prefer Helm, but please do use caution. I wouldn't be surprised if more work to get the helm resources in sync again is necessary.
helm repo add redis-operator https://spotahome.github.io/redis-operator | ||
helm repo update | ||
helm install redis-operator redis-operator/redis-operator |
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.
The CI process uses the helm/chart-releaser-action1 to publish charts as a
self-contained helm chart repository using Github pages. While our fork does publish the charts to the gh-pages branch github pages remains disabled for this repository.
Let me see about getting that enabled and see if it moves the needle on the viability of installing from the repo.
Footnotes
charts/redisoperator/crds/databases.spotahome.com_redisfailovers.yaml
Outdated
Show resolved
Hide resolved
{{- toYaml . | nindent 4 }} | ||
{{- end }} | ||
controller-gen.kubebuilder.io/version: (devel) | ||
krane.shopify.io/instance-rollout-conditions: "true" |
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.
FWIW,
The CRDs are typically generated via kubebuilder. This particular annotation comes from kubebuilder annotations on the types in types.go.
To further complicate things, the code generator does not managed the helm chart's CRD. The contents here are usually copy-pasta'd from the resource /manifests
, if I recall. I suspect that' hasn't been done for some time and the current state of the helm chart's CRD is likely woefully behind.
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.
Oops. What can we do then? Could you update the CRDs in this PR perhaps?
Hey @indiebrain, thank you for looking into this PR! Admittedly, I omitted the PR description, but the goal was to start using this code for our purposes, in GKE using Helm. These are the changes we chose to apply to fix issues that ocurred during the installation/first use plus improvements to docs and defaults that were not blocking us, but they make the experience better. There is more info in particular commit messages. Update: the truth is that I didn’t put a lot of effort into this PR (yet) as I wasn’t sure if anyone will actually look at it. That’s the sad reality of so many open source projects! So this is just a dump of the commits we applied to our fork to Make It Work For Us ™️. But I’ll be happy to improve it if needed to get it merged and be actually useful for others too! |
after uninstalling the Chart with Helm Thx @indiebrain!
Please see also #76