elixir-cloud-aai / cwl-WES

Trigger CWL workflows via GA4GH WES and TES
Apache License 2.0
16 stars 18 forks source link

Add option to specify app-config.yaml contents in values file for helm #238

Closed zv0n closed 2 years ago

zv0n commented 2 years ago

Is your feature request related to a problem? Please describe. I would like to be able to specify contents of app-config.yaml inside the values file of the Helm chart. Due to the current way of populating the app-config config map (via a job) whenever the helm release is redeployed, all changes to the config map are overwritten. The only way to permanently change the config map is to create a modified wes image with the edited app-config.yaml, I find this rather cumbersome.

Describe the solution you'd like Edit the templates/wes/app-config.yaml file to take input from values rather than wait to be populated by a job.

uniqueg commented 2 years ago

Thanks @zv0n. This sounds very useful. Would you mind expanding a little bit on the solution you'd like? In particular, do you see a way to do this without having a second copy of the config in the Helm chart? Sorry, I have basically no experience with Helm/k8s.

@lvarin, @zagganas - what do you think?

zv0n commented 2 years ago

The solution I'd like is basically the pull request I've made :D - https://github.com/elixir-cloud-aai/cwl-WES/pull/239

I see 2 possible solutions for not keeping a second copy of the config file (there might be solutions that I don't see, these are just ones that came to mind):

I personally think the helm chart would be simpler to understand if the config was kept in values.yaml and the job that fills config map with data was removed altogether, but I understand it could be difficult to ensure that config in values.yaml is always the same as cwl_wes/config/app_config.yaml

uniqueg commented 2 years ago

Awesome - thanks a lot for this detailed info! Instinctively, the second solution you describe seems best to me, but let's see what our devops think. We are busy at the moment because of a deadline next week, but will get to it after.

lvarin commented 2 years ago

Close by #239