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

[DRAFT] Switch helm deploy to util templating functions #9065

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

girstenbrei
Copy link

Fixes: #9062
Related: #9063

Note This is a raw draft. I'm extremely aware that this PR does have no unit tests at all and does not contain any docs update. It is meant as a foundation to discuss an architecture how to achieve the stated goals. As soon as the direction is clear, unit tests + docs are absolutely necessary.

Description

The goal is to have consistent templating available for the helm renderer & deployer. This includes:

  1. Be able to use all available functions when templating (especially default)
  2. Have a consistent set of variables available for templating accros fields

Change overview

To achieve this, a Templater has been added to pkg/skaffold/util/template.go. The intention is to have something to instantiate once which carries along all configuration and a base set of template-able values. After that, a number of templates can be rendered with optional overrides.

Helm specific templating logic can be found in pkg/skaffold/helm/template.go. The goal here is to have one location containing templating logic. This should avoid different stages 'drifting' away from each other (e.g. helm deploy vs. helm render).

All other changes are in pkg/skaffold/helm/*.go to centralize templater instantiation, passing it around and using the new rendering functions.

Known Implementation Drawbacks

  1. Currently, this implementation cannot cope with template caching. A call to render will always parse and render a template in one step. For helm this seems sufficient, but other use cases may have a problem here.
  2. Performance optimize the value looping in Templater.Render. Always looping over all the available values seems wasteful, I just don't know how to solve this best.

Known Architecture Drawbacks

  1. With this architecture is instantiated at least once per deployer & renderer. If those steps could receive an already configured templater, this could be centralized even more. But, this is a heavier change to those interfaces and carries the question how the templater should be configured and who is responsible to add what to the templater.

User facing changes

New features

Users are now able to use every std and sprig template function in every field consistently.
Additionally, which fields are available for templating has been unified to have them all available in every already template-able field.

UX Changes

Previously, templating was done on demand. This means, during a deploy, release of helm chart A might be deployed, but during release of helm chart B templating failed, making the whole deploy fail in the middle.

Templating of every chart has been moved in front deploying. This means in contrast to before, templating has to succeed for all releases before any deploy is attempted. IMHO this is a better solution than to fail in the middle.

Follow-up Work

Many other parts of skaffold template, too. With the code in this PR, templating logic is now split in pkg/skaffold/util/env_tempalte.go and pkg/skaffold/util/template.go in an effort to start with one part but be able to continue switching other templating to the new structure, too.

IMHO regardless of renderer/deployer/tagger, templating should work consistently inside skaffold. To achieve this, having one central piece of templating logic might be a solution to be discussed.

@google-cla
Copy link

google-cla bot commented Sep 4, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@conventional-commit-lint-gcf
Copy link

conventional-commit-lint-gcf bot commented Sep 4, 2023

🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use automerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant