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

[bitnami/kafka] #25646 Kafka config with yaml parameter map #26342

Merged

Conversation

rubenvw-ngdata
Copy link
Contributor

Description of the change

Use a yaml parameter map instead of a flat file for the extraConfig parameter. This allows for easier overloading both between the default and broker / controller values; and between multiple values.yaml.

Benefits

You can now overload config values easily.

Applicable issues

Checklist

  • Chart version bumped in Chart.yaml according to semver. This is not necessary when the changes only affect README.md files.
  • Variables are documented in the values.yaml and added to the README.md using readme-generator-for-helm
  • Title of the pull request follows this pattern [bitnami/<name_of_the_chart>] Descriptive title
  • All commits signed off and in agreement of Developer Certificate of Origin (DCO)

@github-actions github-actions bot added the triage Triage is needed label May 22, 2024
@github-actions github-actions bot requested a review from carrodher May 22, 2024 14:30
@rubenvw-ngdata rubenvw-ngdata force-pushed the ISSUE-25646_kafka_config_yaml_parameter_map branch from e73aa2b to daf67b3 Compare May 22, 2024 14:33
@carrodher carrodher added verify Execute verification workflow for these changes in-progress kafka labels May 22, 2024
@github-actions github-actions bot removed the triage Triage is needed label May 22, 2024
@github-actions github-actions bot removed the request for review from carrodher May 22, 2024 16:09
@github-actions github-actions bot requested a review from migruiz4 May 22, 2024 16:09
Copy link
Member

@migruiz4 migruiz4 left a comment

Choose a reason for hiding this comment

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

Hi @rubenvw-ngdata,

Thank you for your contribution!

As mentioned in my comments, I would appreciate it if you could consider adding this feature as a new value instead of introducing a non-backward compatible change.

bitnami/common/templates/_propertiesvalues.tpl Outdated Show resolved Hide resolved
bitnami/kafka/Chart.yaml Outdated Show resolved Hide resolved
bitnami/common/templates/_propertiesvalues.tpl Outdated Show resolved Hide resolved
bitnami/kafka/values.yaml Outdated Show resolved Hide resolved
@rubenvw-ngdata rubenvw-ngdata force-pushed the ISSUE-25646_kafka_config_yaml_parameter_map branch 2 times, most recently from f8db216 to 2e017e1 Compare May 30, 2024 13:17
@rubenvw-ngdata
Copy link
Contributor Author

rubenvw-ngdata commented May 30, 2024

@migruiz4 Still a bit in doubt whether supporting both is better and if there is any advantage using the 'old' extraConfig at all.
In most strings, the quotes will not be required, so that is not a valid reason for me.

Please go ahead and review and merge this if you agree with the changes as you proposed them.

if I convinced you of dropping the old extraConfig property, and I will make the changes for that.

@rubenvw-ngdata rubenvw-ngdata requested a review from migruiz4 May 30, 2024 13:19
Copy link
Member

@migruiz4 migruiz4 left a comment

Choose a reason for hiding this comment

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

Thank you very much for applying my suggestions!

I see the tpl I suggested was causing some issues. I tested your branch locally and using common.tplvalues.render should fix the issue.

bitnami/kafka/Chart.yaml Outdated Show resolved Hide resolved
@rubenvw-ngdata rubenvw-ngdata force-pushed the ISSUE-25646_kafka_config_yaml_parameter_map branch from 2e017e1 to e3efb1e Compare May 30, 2024 13:33
Allow to use a yaml parameter map next to the flat file for the
extraConfig parameter. This is added as a separate parameter
extraConfigYaml. This allows for easier overloading both between
the default and broker / controller values; and between multiple
values.yaml.

Signed-off-by: Ruben Van Wanzeele <[email protected]>
@rubenvw-ngdata rubenvw-ngdata force-pushed the ISSUE-25646_kafka_config_yaml_parameter_map branch from f6d8078 to b8fb584 Compare May 30, 2024 13:35
@rubenvw-ngdata rubenvw-ngdata requested a review from migruiz4 May 30, 2024 13:36
@migruiz4
Copy link
Member

migruiz4 commented May 30, 2024

Regarding your comment, the reason to keep both extraConfig and extraConfigYaml is mostly to not introduce a non-backward compatible change, and therefore force a new major.

Additionally, with the plaintext extraConfig you could technically use templates at a higher level, for example:

extraConfig: |
   {{- $iterator := list "a" "b" "c" }}
   {{- range $i := $iterator }}
   kafka.setting.$i=false
   {{-end}}
   {{- if .Values.metrics.enabled }}
   ...
   {{- else }}
   ...
   {{- end }}

Can not ensure anyone is using the value extraConfig at that level, but by replacing it we would be removing this feature and forcing users to perform manual action to migrate to the new YAML object format.

Copy link
Member

@migruiz4 migruiz4 left a comment

Choose a reason for hiding this comment

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

Thank you very much for your contribution!

@migruiz4 migruiz4 merged commit 4a023a2 into bitnami:main May 30, 2024
9 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kafka solved verify Execute verification workflow for these changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bitnami/kafka] add support for helm parameter maps for extraConfig
3 participants