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

remove hook to be helm3 compatible, change crd generation, move to crds directory #441

Closed
wants to merge 4 commits into from

Conversation

21h
Copy link
Contributor

@21h 21h commented Dec 17, 2019

crd hook not supported in helm3, tested installation without this hook and all works fine.

Copy link
Contributor

@AMecea AMecea left a comment

Choose a reason for hiding this comment

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

Hi @21h, thank you for opening this PR!

This will brake helm 2 compatibility. My suggestion is to do as guys from Prometheus operator did. Doing so will be backward compatible.

Those files are generated, so you have to change the script that updates them. See this file.

@21h 21h changed the title remove hook remove hook to be halm3 compatible, change crd generation, move to crds directory Dec 31, 2019
@21h 21h changed the title remove hook to be halm3 compatible, change crd generation, move to crds directory remove hook to be helm3 compatible, change crd generation, move to crds directory Dec 31, 2019
@21h 21h requested a review from AMecea December 31, 2019 15:39
@21h
Copy link
Contributor Author

21h commented Dec 31, 2019

@AMecea checkout files, I made changes you requested

Copy link
Contributor

@AMecea AMecea left a comment

Choose a reason for hiding this comment

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

Thank you @21h !

It's almost done, you have to add a file template/crd.yaml which will include the CRDs for helm version 2. See this code.

# templates/crds.yaml update
awk 'FNR==1 && NR!=1 {print "---"}{print}' ${CONFIG_PATH}/crds/*.yaml > ${CHART_PATH}/templates/_crds.yaml
yq m -d'*' -i ${CHART_PATH}/templates/_crds.yaml chart-metadata.yaml
yq w -d'*' -i ${CHART_PATH}/templates/_crds.yaml 'metadata.annotations[helm.sh/hook]' crd-install
Copy link
Contributor

Choose a reason for hiding this comment

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

the annotations helm,sh/hook=crd-install should be kept to be backward compatible.

@@ -23,14 +16,8 @@ apiVersion: apiextensions.k8s.io/v1beta1
kind: CustomResourceDefinition
metadata:
labels:
app: '{{ template "mysql-operator.name" . }}'
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest keeping this label to a hardcoded value: app=mysql-operator

@AMecea
Copy link
Contributor

AMecea commented Jan 6, 2020

Also, please update the chart README.md and also bump chart version, see #445

@AMecea
Copy link
Contributor

AMecea commented Jan 15, 2020

ping @21h 😄

@vitobotta
Copy link
Contributor

Hi @21h and @AMecea , I have tried several times with the changes shown in this PR, and I can install the operator with these changes, but backups do not work for some reason. If I install with Helm 2, backups work just fine. @21h Have you tested backups with your changes? Thanks!

@vitobotta
Copy link
Contributor

I have a doubt.... I have only made the changes to the CRD file and moved it to the crds directory. Do I need to use the script generate_chart_manifests.sh as well? Thanks

@vitobotta
Copy link
Contributor

In the logs for the failing backup jobs I see this:

sidecar take backup command failed      {"error": "getting backup: fail to get backup: Get http://mysql-cluster-mysql-0.mysql.mysql:8080/xbackup: dial tcp: i/o timeout, code: unknown"}

I tried to curl Is mysql-cluster-mysql-0.mysql.mysql:8080 from a Ubuntu container in the same namespace and I could connect. I wonder why the backup job cannot connect to it?

@vitobotta
Copy link
Contributor

I found my problem.... Even though I checked out the 0.3.8 tag, the images in the values.yaml are set to "latest", instead of "0.3.8". Changing that fixes the backups.

@AMecea AMecea mentioned this pull request Feb 11, 2020
@AMecea
Copy link
Contributor

AMecea commented Feb 11, 2020

Closing this in favor of #468. In #468 I've cherry-picked your commits and squashed them and add the rest of changes. Please test it and let me know it solves your problem. Thank you for your contribution!

@AMecea AMecea closed this Feb 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants