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

Update affinity definition for tpl usage #660

Closed
wants to merge 4 commits into from
Closed

Update affinity definition for tpl usage #660

wants to merge 4 commits into from

Conversation

goyertp
Copy link

@goyertp goyertp commented Sep 1, 2023

Pull request description

This solution cannot utilize the toYaml function due to its dependence on dynamic TPL functionality for passing selector labels to affinity rules.

Using the tpl function in Helm instead of toYaml can be necessary in certain scenarios due to the specific requirements and limitations of Helm and Kubernetes configurations:

  1. Dynamic Templating: Helm's tpl function allows for dynamic templating, enabling you to generate configurations by interpolating values or variables. This can be useful when you need to create configuration elements programmatically, such as dynamically defining affinity rules or generating labels based on input values.

  2. Parameterization: Helm is designed to make it easy to parameterize and customize Kubernetes configurations. By using tpl, you can incorporate Helm values and templates into your configuration, making it more flexible and adaptable to different environments or use cases.

  3. Complex Logic: In some cases, you may need to apply complex logic or calculations to generate configuration elements. tpl allows you to include Helm-specific functions and expressions within your templates to achieve this, which may not be straightforward or even possible using toYaml.

  4. Compatibility: Helm templates are a Helm-specific way to generate Kubernetes manifests. While toYaml can convert Helm templates to YAML, using tpl ensures that your templates remain Helm-centric and compatible with Helm's rendering and deployment process.

However, there may still be situations where toYaml is useful, such as when you want to convert a Helm chart to plain YAML for debugging, manual inspection, or use outside of Helm. It's important to choose the appropriate tool based on your specific requirements and whether you need the dynamic templating capabilities provided by tpl or the YAML conversion offered by toYaml.

Checklist

  • I have added unit tests
  • [ X ] I have applied the change to all applicable products
  • [ X ] The E2E test has passed (use e2e label)

@atlassian-cla-bot
Copy link

atlassian-cla-bot bot commented Sep 1, 2023

Hooray! All contributors have signed the CLA.

@bianchi2
Copy link
Collaborator

bianchi2 commented Sep 4, 2023

@goyertp while this suggestion does make sense I am worried about backward compatibility as it's going to be a breaking change for customers who have defined affinity in their values as map[string]interface {}. Helm upgrade will fail.

It looks like identifying data type and introducing a new condition makes it backward compatible:

{{- with .Values.affinity }}
affinity:
  {{- if kindIs "string" $.Values.affinity }}
  {{- tpl . $ | nindent 8 }}
  {{- else }}
  {{- toYaml . | nindent 8}}
  {{- end }}
{{- end }} 

Does this seem reasonable to you?

@goyertp
Copy link
Author

goyertp commented Sep 4, 2023

Your comment is important! As you correctly mention, it is a breaking change that would potentially affect other customers.

Your adjustment in the chart is valid!

Copy link
Collaborator

@bianchi2 bianchi2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is a breaking change, the PR needs to address backwards compatibility.

@bianchi2
Copy link
Collaborator

@goyertp I'll close this PR as it needs more work. Feel free to reopen. Please make sure that changes are backward compatible.

@bianchi2 bianchi2 closed this Oct 12, 2023
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