search

epics-containers / edge-containers-cli

command line shortcuts for epics containers developers
Apache License 2.0
3 stars 1 forks source link

deploy-local override entrypoint with sleep #149

Closed marcelldls closed 1 month ago

marcelldls commented 1 month ago

Requires https://github.com/epics-containers/ec-helm-charts/pull/15

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 83.33333% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 84.55%. Comparing base (11eca91) to head (a3a0999).

Files Patch % Lines
src/edge_containers_cli/cmds/k8s_commands.py 66.66% 1 Missing :warning:
src/edge_containers_cli/cmds/local_commands.py 85.71% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #149 +/- ## ========================================== - Coverage 84.69% 84.55% -0.15% ========================================== Files 15 15 Lines 843 848 +5 ========================================== + Hits 714 717 +3 - Misses 129 131 +2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

marcelldls commented 1 month ago

Does this PR require a change to the helm chart?

All looks good.

Yes, the proposed change is linked. Although I think its fair to say the default values (entry point in this case) have not been done the same as the rest of our parameters - where we have set the defaults in the chart values.yaml - and not the template like I have done here.

I feel like the required entrys in the beamline shared values.yaml should be the things that are beamline specific. Perhaps key-values pairs like iocFolder: /epics/ioc should have defaults set in the ioc-instance template. In this way they are still override-able but its not required per beamline

marcelldls commented 1 month ago

Does this PR require a change to the helm chart? All looks good.

Yes, the proposed change is linked. Although I think its fair to say the default values (entry point in this case) have not been done the same as the rest of our parameters - where we have set the defaults in the chart values.yaml - and not the template like I have done here.

I feel like the required entrys in the beamline shared values.yaml should be the things that are beamline specific. Perhaps key-values pairs like iocFolder: /epics/ioc should have defaults set in the ioc-instance template. In this way they are still override-able but its not required per beamline

Does this PR require a change to the helm chart? All looks good.

Yes, the proposed change is linked. Although I think its fair to say the default values (entry point in this case) have not been done the same as the rest of our parameters - where we have set the defaults in the chart values.yaml - and not the template like I have done here.

I feel like the required entrys in the beamline shared values.yaml should be the things that are beamline specific. Perhaps key-values pairs like iocFolder: /epics/ioc should have defaults set in the ioc-instance template. In this way they are still override-able but its not required per beamline

There might be something strange happening. When I read the Helm documentation, it seems that the subchart values.yaml should be the default - however - when I tried this then these values were ignored. This is why I ended up putting it in the template

@gilesknap should this not be flat? https://github.com/epics-containers/ec-helm-charts/blob/4328ce6e933a7cd60e97747085f18e998991384d/Charts/ioc-instance/values.yaml#L1-L8

marcelldls commented 1 month ago

We have decided to fix the issue by allowing override through exposing of the entry point in the sub chart