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

fix: Allow overwriting of rendered template in merlin and turing charts #361

Merged
merged 5 commits into from
Oct 9, 2023

Conversation

shydefoo
Copy link
Contributor

@shydefoo shydefoo commented Oct 5, 2023

Motivation

  • Current implementation of rendering templates of merlin and turing using render field is incomplete. This MR introduces a new field overwrites, to overwrite specific fields that the render produces.

Modification

  • New render.overwrites field to overwrite portions of the rendered template
  • If render is empty, falls back to original mechanism of using .Values.config and $defaultConfig

For example, the current implementation does not allow the following config to be rendered in merlin:

      BaseImages:
        3.7.*:
          ImageName: ghcr.io/caraml-dev/merlin/merlin-pyfunc-base-py37:0.34.0-rc1
          DockerfilePath: "python/pyfunc-server/docker/Dockerfile"
          BuildContextURI: "git://github.com/caraml-dev/merlin.git#refs/tags/v0.34.0-rc1"
        ...
        # Test MLFlow upgrade
        3.10.*:
          ImageName: ghcr.io/caraml-dev/merlin/merlin-pyfunc-base-py310:0.0.0-f9fda74f934ebf546e4581495d93120e37f6a36b
          DockerfilePath: "python/pyfunc-server/docker/Dockerfile"
          BuildContextURI: "git://github.com/caraml-dev/merlin.git#refs/pull/465"

without setting rendered to an empty dictionary. This is because the existing config will be overwritten by the rendered template if rendered is set.

In order to get around this, a new field overwrites is added to the rendered dictionary, which allows users to specify which fields in the rendered template should be overwritten. The same can be applied to turing.

overwrites is like an escape hatch, it's not expected to be used except for testing purposes.
defaultConfig uses some values from .Values.config which makes it quite hard to use .Values.config to overwrite the rendered template sometimes, and have the rendered template overwrite other parts

Checklist

  • Chart version bumped
  • README.md updated

@shydefoo shydefoo requested a review from a team as a code owner October 5, 2023 05:44
@shydefoo shydefoo changed the title fix: Use merge overwrite fix: Use merge overwrite [DRAFT] Oct 5, 2023
@shydefoo shydefoo changed the title fix: Use merge overwrite [DRAFT] fix: Allow overwriting of rendered template in merlin and turing charts Oct 5, 2023
charts/merlin/README.md Outdated Show resolved Hide resolved
@mbruner mbruner self-requested a review October 6, 2023 04:09
@shydefoo shydefoo merged commit 4656b54 into main Oct 9, 2023
3 checks passed
@shydefoo shydefoo deleted the use-merge-overwrite branch October 9, 2023 10:24
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.

2 participants