search

drupalwxt / helm-drupal

Helm Chart for deploying an enterprise-grade Drupal environment.
https://drupalwxt.github.io/helm-drupal/index.yaml
MIT License
31 stars 22 forks source link

Extra DB availability script #122

Closed bernardmaltais closed 2 years ago

bernardmaltais commented 2 years ago

Adding the ability to provide a custom MySQL DB Availability script. The current one in the chart is so verbose that it causes issue with looking at logs in Kibana. Keeping it much less verbose via a custom script will significantly improve log readability.

zachomedia commented 2 years ago

Thanks @bernardmaltais! I wonder if we should just always make it a value in values.yaml and set the default to current script? Thoughts @bernardmaltais and @sylus? Then perhaps we could call the setting dbAvailabilityScript

bernardmaltais commented 2 years ago

@zachomedia I tried to make the update "transparent" to existing users. If it was implemented as you proposed would it not result in a breaking update for users? Hummm... I see... it would take it from the internal values.yaml file as the default rather then use the if statement if the chart... Yeah... it would probably be cleaner code.

I don't have an issue with it. Either options would work for me.

The more I look at it the more I think the default approach is best because it is not an extra DB Availability script but rather a substitution... I was wrong to use the extra Script naming approach...

In fact many other parts of the job scripts could benefit from defaults substitution in the values file. Many times I wished I could customize the backup or restore jobs on a per env basis but the way the chart is currently written it is hard coded... so I think much more work could be done here to allow better customization across the jobs scripts for certain sections.

Once it is decided what is best moving forward I could start to submit updates to allow customization for the different sections of the jobs.

zachomedia commented 2 years ago

In fact many other parts of the job scripts could benefit from defaults substitution in the values file. Many times I wished I could customize the backup or restore jobs on a per env basis but the way the chart is currently written it is hard coded... so I think much more work could be done here to allow better customization across the jobs scripts for certain sections.

I think this is a great idea! It was, honestly, built initial for a specific use case but I think it can greatly benefit from being expanded and more customizable. Even other stuff like the migrations that are executed would benefit in being customizable. Default values via the values.yaml I think is the best and cleanest approach.

Once it is decided what is best moving forward I could start to submit updates to allow customization for the different sections of the jobs.

I'll let @sylus jump in with his thoughts before we finalize on a direction for this change.

bernardmaltais commented 2 years ago

I converted the code to use the internal values.yaml instead. Much cleaner and should be super easy for users to customize.

sylus commented 2 years ago

Yup I completely agree here and think would let some of the charts end users to better override and customize their settings but showing a nice default. +1 from me :)

bernardmaltais commented 2 years ago

@sylus Sound good. I knew you would land on the values file so I already made the changes ;-) How would you recommend we modify the rest of the script to support values.yaml customization? Create a separate pull request with all the changes bundled into one update? Or should I just piggy back on this one to add the other ones?

bernardmaltais commented 2 years ago

Hummm... this could run pretty deep... I can see that allowing customization could have possibly a profound impact... for example:

              echo "Drop database"
              drush sql-drop -y
              echo "Restore database"
              gunzip -c /backup/$BACKUPNAME/db.sql.gz{{ if .Values.drupal.restore.convert }} | sed 's/MyISAM/InnoDB/g'{{ end }} | drush sql-cli
              echo "Database restored"

This whole block does an "if" that could be totally removed since it is only used in this one script... The .Values.drupal.restore.convert could then be removed from the config file... So instead of doing it this way it could be folded as a script customization... but this would be a breaking change... I wonder if the values.yaml file can contain the actual if statement with values reference... would this be properly expanded by the chart? Therefore this whole block could be moved to the values.yaml file as is and would not result in a breaking change?

I guess we need to discuss how we add customization to the different blocks... One size fits all might not work here.

bernardmaltais commented 2 years ago

I did a quick test and apparently helm will not render things like {{- if .Values.external.enabled }} received as part of a value... so this will limit what can be done from a script customization perspective... pretty much limiting customization to script blocks that do not include yaml syntax... Long discussion here: https://github.com/helm/helm/issues/2133

bernardmaltais commented 2 years ago

Any update on this one? Can it be merged and more updates like this be applied via future PR?

zachomedia commented 2 years ago

@bernardmaltais Apologies for the delay.. I think this is looking good, I have just one question that I put on the MR. Once we decide one way or another I'm happy to merge this and accept future updates :)

bernardmaltais commented 2 years ago

@zachomedia Sound good. I have updated the code with the new value name and revved up the chart version.